[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:
> > 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.
> Eli, I think you're missing the point slightly.  For the induction step to be zero, and for this to be the sole exit, then the loop must be infinite.  Since mustprogress infinite loops are undefined, any result from this function is allowed.  If there's a divide by zero which causes a compiler crash, that would be bad, but literally any result is legal at that point.  
If the induction step is zero, and this is the sole exit, the loop is either infinite, or has a backedge-taken count of zero.  See, for example, https://godbolt.org/z/9djfj1Ycq .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104140