[PATCH] D104140: [SCEV] Allow negative steps for LT exit count computation

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11462
     //
-    // We want to make sure that the stride is truly unknown as there are edge
-    // cases where ScalarEvolution propagates no wrap flags to the
-    // post-increment/decrement IV even though the increment/decrement operation
-    // itself is wrapping. The computed backedge taken count may be wrong in
-    // such cases. This is prevented by checking that the stride is not known to
-    // be either positive or non-positive. For example, no wrap flags are
-    // propagated to the post-increment IV of this loop with a trip count of 2 -
-    //
-    // unsigned char i;
-    // for(i=127; i<128; i+=129)
-    //   A[i] = i;
-    //
-    if (PredicatedIV || !NoWrap || isKnownNonPositive(Stride) ||
-        !loopIsFiniteByAssumption(L))
+    if (PredicatedIV || !NoWrap || !loopIsFiniteByAssumption(L))
       return getCouldNotCompute();
----------------
reames wrote:
> efriedma wrote:
> > Does the logic here actually work correctly for step=0?  It looks like we end up dividing by zero.
> > 
> > If we can prove the step is non-zero, loopIsFiniteByAssumption seems too aggressive; a lack of abnormal exits should be enough.  We'll hit UB after a finite number of steps.
> > 
> > Using isKnownPositive doesn't really make sense in the unsigned case; an unsigned number can't be negative.
> Step can't be zero, or the loop would be infinite.  Though, actually, writing that, I see a latent bug here.  The loop could be finite *because we took another exit*, the step of this IV could still be zero, and we could still have a divide by zero along this path.
> 
> I don't think that has anything to do with this change though.
> 
> p.s. Yes, we can infer non-zero other ways.  I even have one patch out to do that right now.  :)
I agree this patch makes sense, just saying it might expose other issues.

> *because we took another exit*

Even if we take this exit.  If the backedge-taken count is zero, the step doesn't matter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104140/new/

https://reviews.llvm.org/D104140