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

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM

But like I mentioned, I'd like to see more test coverage at some point.



================
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)
 
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105942