[PATCH] D108601: [SCEV] Infer nsw/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);
----------------
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.)


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

https://reviews.llvm.org/D108601