-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: Fold (x + c1) + c2
in lowering
#108779
Conversation
This transformation already exists in morph, but this duplicates the transformation in lowering to facilitate removing the call to morph from CSE. This transformation happening after CSE is somewhat important because shared constant CSE can introduce this IR shape.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
BlockRange().Remove(op1); | ||
BlockRange().Remove(op2); | ||
node->BashToConst(op1->AsIntCon()->IconValue() + op2->AsIntCon()->IconValue(), node->TypeGet()); | ||
int64_t c1 = op1->gtGetOp2()->AsIntConCommon()->IntegralValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably save several lines of code to do gtFoldExprConst(gtNewOperNode(GT_ADD, op1->gtGetOp2(), op2))
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, I'll keep that in mind for the future
if (comp->opts.OptimizationEnabled() && op1->IsCnsIntOrI() && op2->IsCnsIntOrI() && !node->gtOverflow() && | ||
(op1->IsIconHandle(GTF_ICON_OBJ_HDL) || op2->IsIconHandle(GTF_ICON_OBJ_HDL)) && | ||
!op1->AsIntCon()->ImmedValNeedsReloc(comp) && !op2->AsIntCon()->ImmedValNeedsReloc(comp)) | ||
if (comp->opts.OptimizationEnabled()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be relaxed to Tier0OptimizationEnabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me relax this in a separate PR since it will also enable the existing opt in tier 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits, feel free to ignore
This transformation already exists in morph, but this duplicates the transformation in lowering to facilitate removing the call to morph from CSE. This transformation happening after CSE is somewhat important because shared constant CSE can introduce this IR shape.
This should address a number of regressions seen for arm64 in #106695.