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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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.
----------------
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.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11508
+        // FIXME: Signed equivalent?
+        if (!IsSigned && EndC->getAPInt().ule(-StrideC->getAPInt()))
+          return false;
----------------
reames wrote:
> This appears to hold, though I didn't follow the logic TBH.  I resorted to writing a small program to brute force the i8 space.  :)
> 
> Please state the !IsSigned precondition in the comment in End >=u Start form.  
> 
> 
It's basically just proving "end + (stride-1)" doesn't overflow.  I'll try to clarify the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105216