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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:705
+                               ScalarEvolution &SE) {
+  if (SE.isKnownNonNegative(BoundSCEV))
+    return true;
----------------
I don't think that these two checks make any sense, because `isLoopEntryGuardedByCond` makes trivial checks anyways. You are just duplicating efforts.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:712
+  return (SE.isLoopEntryGuardedByCond(L, ICmpInst::ICMP_SGE, BoundSCEV,
+                                      SE.getConstant(Ty, 0, true)) ||
+          SE.isLoopEntryGuardedByCond(L, ICmpInst::ICMP_UGE, BoundSCEV,
----------------
Are different values of `IsSigned` flag really needed here? :) You could've created zero constant just once.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:936
     // not wrap.  This restriction can potentially be lifted in the future.
-
-    if (!HasNoSignedWrap(AR))
+    if (!HasNoSignedWrap(AR) && !AR->hasNoUnsignedWrap())
       return false;
----------------
This does not feel right. What if the indvar goes from `SINT_MAX - 10` to `SINT_MIN + 10` and, thus, has signed wrap? In this case it might have `nuw`, but if we deal with signed predicates, we might miscompile.

I think the correct approach would be to identify whether the latch predicate is signed or unsigned, and if it is unsigned, then we may check `nuw` instead of `nsw`.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:972
       // 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) {
----------------
If these two changes (i.e. checking of `nuw` flag and advanced non-negative) check can be separated into 2 patches, plase do.


https://reviews.llvm.org/D45439