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

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:705
+                               ScalarEvolution &SE) {
+  if (SE.isKnownNonNegative(BoundSCEV))
+    return true;
----------------
mkazantsev wrote:
> I don't think that these two checks make any sense, because `isLoopEntryGuardedByCond` makes trivial checks anyways. You are just duplicating efforts.
Removing isKnownNonNegative has no effects on the tests, but removing isKnownNegative is causing eq_ne and non_known_positive_end tests to fail. I will investigate.


================
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,
----------------
mkazantsev wrote:
> 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;
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > mkazantsev wrote:
> > > 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`.
> > BTW, why it is `&&` and not `||`? I think you were extending the scope of optimization, not narrowing it.
> Ah, forget it, it's `!`... Scratch my 2nd comment on that.
Good point!


https://reviews.llvm.org/D45439