[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:
> 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.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105942