[PATCH] D45439: [IRCE] Use NUW flag for indvar

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:928
     // not wrap.  This restriction can potentially be lifted in the future.
+    if (HasNoSignedWrap(AR) ||
+        (!IsSigned && AR->hasNoUnsignedWrap())) {
----------------
Why not `(Signed && HasNoSignedWrap(AR)) || (!IsSigned && AR->hasNoUnsignedWrap())`?



================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:929
 
-    if (!HasNoSignedWrap(AR))
-      return false;
----------------
Actually I look into it and the old implementation seems suspicious to me. We only check `nsw`, but when we don't have `nuw` and actually do have an unsigned wrap, why don't we have a bug on unsigned latch predicates? Or we do, but not aware about?.. :)


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:929
+    if (HasNoSignedWrap(AR) ||
+        (!IsSigned && AR->hasNoUnsignedWrap())) {
 
----------------
mkazantsev wrote:
> Actually I look into it and the old implementation seems suspicious to me. We only check `nsw`, but when we don't have `nuw` and actually do have an unsigned wrap, why don't we have a bug on unsigned latch predicates? Or we do, but not aware about?.. :)
If you look at `HasNoSignedWrap`, it has a bit more complex check than just looking into the immediate flag, it also tries to go through `sext` to find this flag. Can we do the same for `nuw` and `zext` to keep things consistent?


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:964
       // Try to turn eq/ne predicates to those we can work with.
-      if (Pred == ICmpInst::ICMP_NE && LatchBrExitIdx == 1)
+      if (Pred == ICmpInst::ICMP_NE && LatchBrExitIdx == 1) {
         // while (++i != len) {         while (++i < len) {
----------------
It seems to be an independent change, please commit it separately as NFC.


================
Comment at: test/Transforms/IRCE/variable-loop-bounds.ll:328
 
+; CHECK-LABEL: signed_var_imm_dec_eq
 define void @signed_var_imm_dec_eq(i32* nocapture %a, i32* nocapture readonly %b, i32* nocapture readonly %c, i32 %M) {
----------------
Same, this can go as NFC without approval.


================
Comment at: test/Transforms/IRCE/variable-loop-bounds.ll:361
+
+; CHECK-LABEL: test_inc_eq_var_nuw(
+; CHECK: main.exit.selector:
----------------
Please add `CHECK` or `CHECK-NOT` on existence/non-existence of preloop and postloop, so that we can make sure that IRCE does good job generating/not generating them.

Also the same applies to the tests above, you can add these checks as NFC separately (not super-urgent, but useful).


https://reviews.llvm.org/D45439