[PATCH] D103844: [SCEV] Cache wrap facts for positive IVs w/LT exits

[Philip Reames via Phabricator via llvm-commits]

reames planned changes to this revision.
reames added a comment.

Change appears to be wrong as written, discussing why before adjusting.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:11518
+    // We have proven that IV can not overflow, remember that fact.
+    setNoWrapFlags(const_cast<SCEVAddRecExpr *>(IV), WrapType);
   }
----------------
nikic wrote:
> nikic wrote:
> > reames wrote:
> > > nikic wrote:
> > > > If we consider the `canIVOverflowOnLT()` case, the only thin this guarantees is that `RHS + Stride - 1` does not overflow, it doesn't make a direct statement about the addrec.
> > > > 
> > > > If the loop exits before reaching this exit (for simplicity: abnormal exit on first iteration), then I don't think we can really make any statement about the nowrap behavior of the addrec.
> > > Right, but we know that the result of the expression being considered must reach a conditional branch, and that conditional branch must dominate the latch.  Given that, we know that if this condition did overflow, then the iteration of the loop would be UB unless we exit from some other exit before this one.  In either case, that seems to give an upper bound on the trip count which does imply that AddRec can't overflow in a well defined loop doesn't it?
> > Ah yes, you're right. It does still provide an upper bound. I got this mixed up with concerns from D101722. I've played around with some examples and convinced myself that this is correct...
> After thinking about it some more, there is still one case that may be problematic. What if the addrec starts out above the RHS (meaning the loop exits after the first iteration). I'm thinking something like this:
> 
> ```
> ; Assume %start is larger than 200.
> define void @test(i8 %start) {
> entry:
>   br label %loop
> 
> loop:
>   %iv = phi i8 [ %start, %entry ], [ %iv.next, %loop ]
>   %iv.next = add i8 %iv, 2
>   call void @use_and_exit(i8 %iv.next)
>   %cond = icmp ult i8 %iv.next, 200 
>   br i1 %cond, label %loop, label %exit
> 
> exit:
>   ret void
> }
> 
> declare void @use_and_exit(i8)
> ```
> 
> I believe your change would now infer that `{2 + %start,+,2}` is `<nuw>`, but if `@use_and_exit()` exits beforehand that is not necessarily a given.
I think you're correct, and that this change is wrong.  But I think the reasoning on why is bit different than your phrasing.  Let me try to describe why, and you can tell me if this matches what you're thinking.

In order for the flags computed by canIVOverflowOnLT to hold for the IV in general, we need to know that a poison value for the comparison must reach a branch (or other full UB use).  If after poison has been generated, but before the undefined use, the loop exits, the overflow flag might not hold.  This can happen when either a) there's an abnormal or normal exit before this one which isn't control dependent on this IV.  If that happens, users of the IV might see a wrapped value even though we've proved this exit can't be taken after overflow.

(Aside, this is specific to the canIVOverflowOnLT path.  I believe the isUBOnWrap path is sufficiently conservative to not have this issue.)

If the IV starts at a value above the RHS, the loop may exit on the first iteration.  But, and this is the key bit, assuming the exit taken is control dependent on the IV, we can still infer the nowrap flag.  Why?  There's two addrecs here.  There's IV = {start, +, 2}, and IV.next = {start + 2, +, 2}.  The exit test is in terms of the later.  Given that, the first iteration of %iv.next can't wrap, or we wouldn't be in the exit on first iteration case.  Hm, I do notice this argument is specific to the step and rhs values here.  If rhs was say, 0, I think this argument collapses.  Maybe we have another latent issue here?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103844