JIT: Move all OperIsConst() to right-side for comparisons (including missing EQ, NE)#127616
JIT: Move all OperIsConst() to right-side for comparisons (including missing EQ, NE)#127616BoyBaykiller wants to merge 2 commits intodotnet:mainfrom
OperIsConst() to right-side for comparisons (including missing EQ, NE)#127616Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
Bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| case GT_EQ: | ||
| case GT_NE: | ||
| // Change "CNS relop op2" to "op2 relop* CNS" | ||
| if (op1->IsIntegralConst() && tree->OperIsCompare() && gtCanSwapOrder(op1, op2)) |
There was a problem hiding this comment.
There is no need to restrict this to IsIntegralConst (same for GE/GT/LE/LT below) and no need to check OperIsCompare either, it's always true for these cases.
We may want to assert OperIsCompare to ensure no accidental codeflow is occurring from future refactorings, but that's optional.
For EQ/NE as well, there's no need to do SwapRelop as there is no change for EQ/NE, again a case that can be asserted but the general helper is for simplicity when the paths are all shared (which isn't trivial to share here)
There was a problem hiding this comment.
I know but:
I just copy-pasted the code from GT_LT, GT_LE, GT_GE, GT_GT because plan is to refactor/unify this anyway.
There was a problem hiding this comment.
But ok let me change it
There was a problem hiding this comment.
There's a difference between doing a big refactoring and doing a small contained improvement consistently.
The other change I had an issue with because it was starting to mix many different fixes and codegen improvements together as well as also mixing in a refactoring as part of that.
This one, however, is propagating a clear issue and so it's not really even fulfilling what the description is advertising.
We still have a small 10-15 line PR doing this "properly" where we fix all of EQ/NE/GT/GE/LT/LE to "canonicalize" for all cases where its valid by pushing constants to the right. It is then just a single logically contained fix for relop nodes.
BoyBaykiller
changed the title
OperIsConst() to right-side for comparisons (including missing EQ, NE)
* specialize handling for NE, EQ
BoyBaykiller
force-pushed
the
EQ-NE-canonicalization-v2
branch
from
e62323e to
6864a7e
Compare
|
Sorry, but I don't understand the intent. I thought we agreed to make it generic for all commutative node, why do we still just copy-paste existing logic? I think we should just handle it in both pre- and post- order before we enter the global switch, we just check if the oper is commutative and check the CanSwap and do it. thus we can also delete a few existing ad-hoc rotations. |
|
Lol. The bikeshedding is real. If someone wants to take over please feel free. I think we all want the same thing in the end, but waste time on how and which way it should be split into PRs or whatever. |
|
Here is the change that I proposed: https://github.com/dotnet/runtime/compare/main...EgorBo:runtime-1:swap-cns?expand=1 (although, probably needs a separate function vs lambda) I assume with this change we can delete a couple of other "move cns to the right" here and there - haven't checked. No copy-paste |
3 participants
When turning 'CNS relop op2' into 'CNS relop* op2' we previously only allowed
IsIntegralConst.Also EQ, NE where not canonicalized at all.