Ensure we preserve vn and produce a canonical tree#127689
Ensure we preserve vn and produce a canonical tree#127689tannergooding wants to merge 2 commits intodotnet:mainfrom
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT morphing to (1) preserve value numbering (VN) across a specific MOD -> AND strength-reduction used under ==/!= 0 comparisons, and (2) canonicalize a SUB -> ADD rewrite so the constant ends up as the second operand.
Changes:
- Preserve VN when rewriting
(a % c) ==/!= 0(power-of-2c) intoa & (c - 1) ==/!= 0. - Adjust the
cns1 - op2rewrite to produce((-op2) + cns1)(constant as operand 2). - Update the in-code comment to reflect the intended canonical tree form.
|
CC. @EgorBo, no diffs but cleans up the pre-existing PreserveVN case you commented on in the other PR |
| } | ||
|
|
||
| /* Check for "cns1 - op2" , we change it to "(cns1 + (-op2))" */ | ||
| /* Check for "cns1 - op2" , we change it to "((-op2) + cns1)" */ |
There was a problem hiding this comment.
I wonder if this transformation even makes any impact and do we revert it back to cns - op2 near codegen 🤔 otherwise we seems to be introducing a new NEG on top of X
There was a problem hiding this comment.
The impact is that adds are associative, while subtracts are not and we have various opts like (x + cns1) + (y + cns2) -> (x + y) + cns3 or (x + cns1) + cns2 to x + cnx3 which we wouldn't find otherwise.
We also tend to not look at subtracts at all for things like address computation or similar; so x - cns would be skipped over while x + (-cns) would not, and same again for cns - x vs (-x) + cns.
We could of course update all those places to handle subtracts as well, and maybe that's even beneficial, but the canonicalization to an add form simplifies a lot of things (at least when constants are involved)
There was a problem hiding this comment.
I wonder if this transformation even makes any impact and do we revert it back to
cns - op2near codegen 🤔 otherwise we seems to be introducing a new NEG on top of X
We don't revert to cns - op2 near codegen. I tried to do so in Lower and it had regressions #126442 (comment). From my understanding problem was register alloc, specifically extra mov.
This ensures that we preserve the VN when morphing a particular
MOD->ANDpattern and ensures that a particularSUB->ADDpattern produces a "canonical" tree with the constant as the second operand.