[SCCP] [Transform] Adding ICMP folding for zext and sext in SCCPSolver #67594
Conversation
llvmbot
added
clang
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| } | ||
|
|
||
|
|
||
| if(Op0->getType() != Op1->getType()){ |
There was a problem hiding this comment.
Its is always better to exit early. This check should ideally be done before potential swapping.
| @@ -0,0 +1,185 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --tool ./bin/opt --version 3 | |||
| ; See PRXXX for more details | |||
There was a problem hiding this comment.
See PRXXX Was that autogenerated ? Does it need to be updated ?
| if (Op1_zext && (!Op0_zext)) { | ||
| // We force Op0 to be a zext and reverse the arguments | ||
| // at the end if we swap | ||
| reversed = true; |
There was a problem hiding this comment.
IMHO the swapping makes it harder to read. Not swapping would force case to be checked in the if statement, but would make the code more readable.
There was a problem hiding this comment.
If I understood correctly, what you're trying to do here is to apply an icmp fold early before the sext -> zext information gets lost. I don't think this is the correct way to approach the problem. The correct way is to preserve the fact that the operand is non-negative when converting to zext, which would allow the InstCombine fold to reliably undo this. In fact, there is a pending patch for this at https://reviews.llvm.org/D156444.
When tackling this problem, my first idea was to track the non-negativeness of the zext and use this information in InstCombine. I didn't know that you could do it through flags in the IR, which I think is a cleaner solution. Do you think this patch will be available soon ? |
|
I'm closing this PR in favour of #70845 |
This PR fixes #55013 : the max intrinsics is not generated for this simple loop case : https://godbolt.org/z/hxz1xhMPh. This is caused by a ICMP not being folded into a select, thus not generating the max intrinsics.
Since LLVM 14, SCCP pass got smarter by folding sext into zext for positive ranges : https://reviews.llvm.org/D81756. After this change, InstCombine was sometimes unable to fold icmp correctly as both of the arguments pointed to mismatched zext/sext. To fix this, @rotateright implemented this fix : https://reviews.llvm.org/D124419 that tries to resolve the mismatch by knowing if the argument of a zext is positive (in which case, it is like a sext) by using ValueTracking. However, ValueTracking seems to be not smart enough for this case and cannot accurately know that the value is positive or not. This PR implements the folding in SCCP directly, where we have the knowledge that the value are positive or not.
This PR also contains test cases for sext/zext folding with SCCP as well as a x86 regression tests for the max/min case.