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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Detailed comments inline.  The two common issues running through are:

- The use of IsSigned confuses me.  We should be computing an unsigned expression at this point, so why does the signedness of the end max expression matter?  I think I finally figured it out but a change to the code structure and comments would really help.   My current understanding is that IsSigned == true implies (End >=s Start) and !IsSigned implies (End >=u Start) as preconditions of the method.  I'd just state that explicitly in comment form.
- We can have an exit which is dynamically dead, produces poison, and overflows in the computation.  See the long explanation in the first comment.  As far as I can tell, this only applies to the first case.

Sorry this took so long.



================
Comment at: llvm/include/llvm/Analysis/ScalarEvolution.h:2027
+  /// 2. If Stride is not positive, End is equal to Start
+  /// 3. The index variable doesn't overflow.
+  ///
----------------
... if *this* exit is taken


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


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11470
+    }
+    // Figure out the "base" start for MayOverflow checks. If the start is
+    // of the form "min(BaseStart, End)", then either Delta is equal to
----------------
Aside: None of the logic below appears to rely on no-wrap facts on the IV.  Note sure if it's worth it, but should we reflect that in code structure somehow?  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11475
+    const SCEV *BaseStart = Start;
+    if (IsSigned) {
+      if (auto *SMinStart = dyn_cast<SCEVSMinExpr>(Start)) {
----------------
Code structure wise, I request you pass in the BaseStart explicitly.  This parsing of expressions is confusing.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11489
+      // If Start is equal to Stride - 1, (End - Start) + Stride - 1 == End.
+      // If IsSigned is true, we assume the stride is positive.
+      return false;
----------------
The last line of this comment confuses me.  Why is this relevant?

Otherwise, the reasoning in this case does appear to hold.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11493
+    if (auto *StrideC = dyn_cast<SCEVConstant>(Stride)) {
+      if (auto *BaseStartC = dyn_cast<SCEVConstant>(BaseStart)) {
+        // Start >= Stride implies End - Start + Stride - 1 < End.
----------------
According to my brute force program, your reasoning does hold.  I find the isSigned usage very very confusing.  Please write out the implied predicates.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11508
+        // FIXME: Signed equivalent?
+        if (!IsSigned && EndC->getAPInt().ule(-StrideC->getAPInt()))
+          return false;
----------------
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.  




================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11514
+      // If Start is non-negative, End - Start is also non-negative. So in
+      // unsigned arithmetic, "(End - Start) + (Stride - 1)" easily fits.
+      return false;
----------------
Again, please state the precondition that IsSigned implies.  Your code is correct, but the comment is very hard to follow.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105216