-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Creating comma temps differently for SubMulDiv morph #69770
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsAt the moment, this is an experiment. This changes how comma temps are created for SubMulDiv morph. Example code: (multiplier * seed2 + increment) % Modulus; In morph, this would turn into: (let tmp1 = multiplier * seed2 + increment) in tmp1) - (tmp1 / Modulus) * Modulus This PR morphs it into: let tmp1 = multiplier * seed2 + increment in
tmp1 - (tmp1 / Modulus) * Modulus
|
…sCloneableInvariantOrLocal
@dotnet/jit-contrib This is ready. The diffs will show some regressions, but they are not detrimental. The improvements on both x64 and ARM64 look good, especially ARM64. |
@kunalspathak @AndyAyersMS - I feel good about this change and it's ready. It should help optimizing 'mod' since I'll be able to do the transformation before rationalization. |
1 similar comment
@kunalspathak @AndyAyersMS - I feel good about this change and it's ready. It should help optimizing 'mod' since I'll be able to do the transformation before rationalization. |
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.
Looks good.
(but: you have formatting to fix).
@TIHan can you show the perf results in this PR? |
Here is the link to the diffs: Diffs |
Seems there is significant regression introduced on arm64:
|
just noticed your message about the regression in #69770 (comment) |
@@ -13833,6 +13877,28 @@ GenTree* Compiler::fgMorphMultiOp(GenTreeMultiOp* multiOp) | |||
// division will be used, in that case this transform allows CSE to | |||
// eliminate the redundant div from code like "x = a / 3; y = a % 3;". | |||
// | |||
// Before: | |||
// * RETURN int |
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.
Something doesn't feel right in Before:
tree. Did it mean to have (V00 * V00) % V01
. Same in After:
.
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.
(V00 * V00) % V01
is intentional.
In the after tree, you see that it takes (V00 * V00)
and hoists it out:
// +--* ASG int
// | +--* LCL_VAR int V03 tmp1
// | \--* MUL int
// | +--* LCL_VAR int V00 arg0
// | \--* LCL_VAR int V00 arg0
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.
As spoke offline, change MUL
to BINOP
and add a note that it is applicable for any BINOP
that is safe to clone.
It does look significant, but when I looked at the top regressions, the actual code is really wild with a lot of casts and checked contexts - something you really would not see in user code. |
That is a good catch. I'm surprised by this one - I'm even more surprised how it was able to CSE it before because one of the ops would have a |
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
My before and after were flipped. I think it looks good. |
As a note from @kunalspathak 's comment regarding the |
Description
This changes how comma temps are created for SubMulDiv morph.
Example code:
In morph, this would turn into:
This PR morphs it into:
Basically, we are moving the comma temps to be the root of the expression instead of leafs.
Acceptance Criteria
Diffs