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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108601