[PATCH] D108601: [SCEV] Infer nuw from nw for addrecs

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:2398
+    if (!ScalarEvolution::hasFlags(Flags, SCEV::FlagNUW) && Ops[0]->isZero() &&
+        IsKnownNonNegative(Ops[1]))
+      Flags = ScalarEvolution::setFlags(Flags, SCEV::FlagNUW);
----------------
nikic wrote:
> nikic wrote:
> > reames wrote:
> > > nikic wrote:
> > > > Why the IsKnownNonNegative check for the NUW case? Shouldn't this be on NSW?
> > > We definitely need it for NSW -  I'd apparently dropped it for the NSW case when rebasing this.  Oops.
> > > 
> > > For the unsigned case, I'll be honest and say I'm not sure.  I get really confused about the interpretation of the signed integer overflow rules on add.  The case I was thinking of was starting at 0, and adding -1.  Does that violate nuw or not?  If it does, we need the positive check.  If it doesn't, we probably don't.  Now that I write this, I'm pretty sure the answer is we don't, but I regularly get confused by that case, so I'll let someone else confirm.  :)
> > > 
> > > Keeping in mind that SCEV and IR mix flag interpretations oddly, I'm strongly tempted to keep the non-negative test for (my) sanity sake even if we don't strictly speaking need it.
> > If you think about it as not adding 0 and -1, but rather 0 and 0xffffffff, then it becomes more obvious that it's nuw. I'd prefer not having an unnecessary isKnownNonNegative check here -- doing sign checks for unsigned arithmetic is usually an indication that some logic isn't right...
> > 
> > (And if we don't need to check the sign, we can drop the `Ops.size() == 2` limitation for the nuw case as well.)
> Okay, looks like I was wrong here! The bit I was missing is that `nw` is actually a signed property:
> ```
>   /// AddRec expressions may have a no-self-wraparound <NW> property if, in
>   /// the integer domain, abs(step) * max-iteration(loop) <=
>   /// unsigned-max(bitwidth).  This means that the recurrence will never reach
>   /// its start value if the step is non-zero.  Computing the same value on
>   /// each iteration is not considered wrapping, and recurrences with step = 0
>   /// are trivially <NW>.  <NW> is independent of the sign of step and the
>   /// value the add recurrence starts with.
> ```
> 
> Note the `abs(step)` in the definition. So we do need to ensure that the step is non-negative, to make sure that `abs(step)` is just `step`.
Yeah, I'd just worked my way to the same conclusion.  For my own future reference, here's the chain of logic.

no-self-wrap is a property which describes the space traversed.  It is not a wrapping property per se.

(0,+, 255) in 8 bit math represents a value which decreases by one on each iteration (by wrapping through unsigned).  Putting NUW on this generates poison on the 2nd iteration.  NW on the other hand, is not violated because the number of values traversed (assuming BTC is small) is less than 255.  

We need the non-negative restriction to line up the restriction in terms of values traversed with the NUW definition.


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

https://reviews.llvm.org/D108601