[PATCH] D105942: [SCEV] Fix unsound reasoning in howManyLessThans

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/test/Analysis/ScalarEvolution/trip-count-unknown-stride.ll:8
 ; CHECK-LABEL: Determining loop execution counts for: @foo1
-; CHECK: backedge-taken count is ((-1 + %n) /u %s)
+; CHECK: backedge-taken count is ((-1 + (%n smax %s)) /u %s)
 
----------------
efriedma wrote:
> reames wrote:
> > efriedma wrote:
> > > reames wrote:
> > > > efriedma wrote:
> > > > > We could actually handle this.
> > > > > 
> > > > > We have "%add = add nsw i32 %i.05, %s" in the IR, feeding into the backedge. We can use this to show "Start > Start - Stride": if it's false, we have poison feeding into the branch.  I don't think we use this sort of reasoning anywhere at the moment, though.
> > > > > 
> > > > > I don't think we need to block this patch for that, necessarily... but we should make sure we have a testcase that can't be rescued by this sort of reasoning.
> > > > This is a good point.  We don't do anything this direct, but we do reason extensively about this code about impossible paths because poison would reach a branch.
> > > > 
> > > > (Note: Your reasoning is only sound when this is the sole exit.  Otherwise, there might be an earlier exit which exits on iteration zero.)
> > > > 
> > > > (It also needs to be a post increment test as well.)
> > > > 
> > > > Now that I think about this, I strongly suspect this is what the original code was trying to do.  It just didn't account for the fact that ControlsExit does not have to be true at this point in code, and doesn't include the test for post-increment.  
> > > > (Note: Your reasoning is only sound when this is the sole exit.  Otherwise, there might be an earlier exit which exits on iteration zero.)
> > > 
> > > If there's an earlier exit which exits on iteration zero, does it matter what we return here?
> > In some ways, yes.  We can return any concrete value for the exit count of the exit, but we could not, for instance, return poison.  At the moment, I don't think we ever actually produce poison, but a more realistic example might be a divide by possible zero.  I think that the expansion code would probably prevent that biting us in practice, but we'd be walking a very thin line.  
> I suspect we need a general solution for the problem of possibly-poison trip counts.  Even the simplest examples have issues.  See, for example, https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=047d8ce24c780675&test=Transforms%2FLoopUnroll%2Fruntime-loop-multiple-exits.ll
Eek, I hadn't realized it was quite *that* broken.   Yeah, we definitely need to figure out something there.  One problem at a time though.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105942