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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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:
> 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`.


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

https://reviews.llvm.org/D108601