[PATCH] D105216: [ScalarEvolution] Fix overflow in computeBECount.

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11455
+      if (StrideC->getAPInt().isPowerOf2()) {
+        // If the stride is a power of two, the maximum backedge-taken count is
+        // is (2^bitwidth / Stride) - 1.  After that, the addrec repeats values.
----------------
efriedma wrote:
> reames wrote:
> > I'm not sure this holds, let me walk you through why.  
> > 
> > There is no guarantee that this exit must be taken.  Instead, some other exit could be taken.
> > 
> > The cornercase here is when we have an exit (which won't be taken) whose no-wrap IV does in fact wrap on the last iteration (producing poison), but whose value we don't branch on (e.g. no UB) because an earlier exit is taken instead. 
> > 
> > I believe this means the minimum BTC we can report for the (untaken) exit is 2^BW/Stride.  (e.g. so it must be >= the exit count of the taken exit)
> > 
> > I believe that invalidates your inequality, and thus your result.
> > 
> > We can patch this in a couple of ways:
> > - If ControlsExit is true, then we know this condition is branched on by the sole exit.  Thus, no other exit, and logic holds.
> > - If we know this condition is branched on by any latch dominating exit, then we know that the execution of iteration 2^BW/Stride must execute UB if no earlier exit is taken.  Thus, either this exit is taken on a previous iteration, or another exit is taken up to that iteration.  (I got stuck here, but it really seems like we should be able to do case analysis?)
> > 
> > Aside: If we already know the other exit count is small, it's a real shame we can't use that here...  Maybe we can do a two step analysis and simplify using min approach?  Just a thought.  
> If ControlsExit is false, and stride isn't one, it's hard to get here. I think the only way is if canIVOverflowOnLT returns false, which I think is enough to prevent any trouble here?  Maybe worth going into in the comment, though.
I believe you are correct, though I'll note I really dislike code whose correctness depends on understanding a non-trivial invariant through a large batch of complicated code.  Please try to comment this clearly!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105216