[PATCH] D15412: [SCEV][LAA] Add no overflow SCEV predicates and use use them to improve strided pointer detection

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:286
@@ +285,3 @@
+    ///
+    /// 0 <= F(a) + G(b) < 2^n
+    ///
----------------
Do you mean `G(a) + F(b)` here?  Might be helpful to name these as `Signed` and `Unsigned` instead of `F` and `G`.

Might be useful to explicitly note that `SCEVWrapPredicate:: IncrementNUW` is **not** commutative, and you're interpreting `{a,+,b}` as `(((a + b) + b) + b) ...` for the purposes of `SCEVWrapPredicate`.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:291
@@ +290,3 @@
+      IncrementAnyWrap = 0,    // No guarantee.
+      IncrementNUW = (1 << 0), // No unsigned wrap.
+      IncrementNSW = (1 << 1), // No signed wrap.
----------------
Please don't call this nuw (here, and elsewhere in comments) -- I don't think having two definitions of nuw float around it the codebase is a good idea.  Pretty much any name other than nuw is fine.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:300
@@ +299,3 @@
+               SCEVWrapPredicate::IncrementWrapFlags OffFlags) {
+      assert(Flags <= IncrementNoWrapMask && Flags >= IncrementAnyWrap &&
+             "Invalid flags value!");
----------------
What you have here is fine, but I'd have done `assert(Flags & IncrementNoWrapMask == Flags)` instead.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9635
@@ +9634,3 @@
+      // This couldn't be folded because the operand didn't have the nuw
+      // flag. Add the nuw flag as an assumption that we could make.
+      const SCEV *Step = AR->getStepRecurrence(SE);
----------------
As mentioned earlier, please don't call this "nuw".

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9733
@@ +9732,3 @@
+
+  return Op->AR == AR && setFlags(Flags, Op->Flags) == Flags;
+}
----------------
Very minor, but why not `return Op && Op->AR == AR && setFlags(Flags, Op->Flags) == Flags;` instead of the early exit above?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9741
@@ +9740,3 @@
+  if (ScalarEvolution::setFlags(ScevFlags, SCEV::FlagNSW) == ScevFlags)
+    IFlags = clearFlags(IFlags, IncrementNSW);
+
----------------
Minor and optional, but why not have an early return here as `return IFlags == IncrementNSW;`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9749
@@ +9748,3 @@
+  if (SCEVWrapPredicate::IncrementNUW & getFlags())
+    OS << "<nuw>";
+  if (SCEVWrapPredicate::IncrementNSW & getFlags())
----------------
As mentioned earlier, please don't call this `<nuw>`.


http://reviews.llvm.org/D15412