[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:278
@@ +277,3 @@
+    /// Similar to SCEV::NoWrapFlags, but with slightly different semantics
+    /// for FlagNUW. The increment is considered to be signed, and a + b
+    /// (where b is the increment) is considered to wrap if:
----------------
I'm still reading through the patch; but if you must introduce a new concept to replace nuw, it should be called something else to avoid confusion.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:280
@@ +279,3 @@
+    /// (where b is the increment) is considered to wrap if:
+    ///    a + b != zext(a) + sext(b)
+    ///
----------------
Do you mean `zext(a + b) != ...`?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:282
@@ +281,3 @@
+    ///
+    /// In the integer domain this is equivalent to 0 <= a + b < 2^n
+    ///
----------------
I'd be a lot more explicit here.  What (I think) you're saying is that

```
0 <= F(a) + F(b) < 2^n
```

where the inequalities and arithmetic above are normal inequalities and arithmetic over integers, and `F` maps an `n` bit tuple to the integer it represents in twos complement.

Is that correct?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:289
@@ +288,3 @@
+      FlagNSW = (1 << 1), // No unsigned wrap.
+      NoWrapMask = (1 << 2) - 1
+    };
----------------
Very minor, but why not `NoWrapMask = FlagNUW | FlagNSW`?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:295
@@ +294,3 @@
+    maskFlags(SCEVWrapPredicate::NoWrapFlags Flags, int Mask) {
+      return (SCEVWrapPredicate::NoWrapFlags)(Flags & Mask);
+    }
----------------
Minor, but what about doing some sanity checking on `Mask` here?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:301
@@ +300,3 @@
+             SCEVWrapPredicate::NoWrapFlags OnFlags) {
+      return (SCEVWrapPredicate::NoWrapFlags)(Flags | OnFlags);
+    }
----------------
Same here and below on `clearFlags`:  some minor sanity checking here would be nice.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:320
@@ +319,3 @@
+  public:
+    SCEVWrapPredicate(const FoldingSetNodeIDRef ID, const SCEVAddRecExpr *AR,
+                      NoWrapFlags Flags);
----------------
`explicit`?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1453
@@ -1369,1 +1452,3 @@
+    /// Records what NoWrap flags we've added to a Value *.
+    DenseMap<Value *, SCEVWrapPredicate::NoWrapFlags> FlagsMap;
     /// The ScalarEvolution analysis.
----------------
Will clients ever try to RAUW / delete `Value` s from under this map.  If not, this should be changed to use an `AssertingVH`; otherwise it needs to use a `CallbackVH` like SCEV.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1457
@@ -1371,1 +1456,3 @@
+    /// The analyzed Loop.
+    Loop &L;
     /// The SCEVPredicate that forms our context. We will rewrite all
----------------
Minor: can you please check if it possible to make this a const reference?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9679
@@ +9678,3 @@
+      // Check if we've already made this assumption.
+      if (P.implies(A))
+        return true;
----------------
Why not just `return P.implies(A);`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9617
@@ +9616,3 @@
+                             ScalarEvolution &SE, SCEVUnionPredicate &A,
+                             bool Assume) {
+    SCEVPredicateRewriter Rewriter(L, SE, A, Assume);
----------------
Please add a comment on what `Assume` means, and use a more descriptive name if possible.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9741
@@ +9740,3 @@
+
+  return Op->AR == AR && Op->Flags == Flags;
+}
----------------
Why not check if `Op->Flags` is a superset of `Flags`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9744
@@ +9743,3 @@
+
+bool SCEVWrapPredicate::isAlwaysTrue() const { return Flags == FlagAnyWrap; }
+
----------------
Might also want to return true if `Flags == NSW && AR->hasNSW()`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9769
@@ +9768,3 @@
+    if (const auto *Step = dyn_cast<SCEVConstant>(AR->getStepRecurrence(SE)))
+      if (Step->isNonConstantNegative())
+        ImpliedFlags = setFlags(ImpliedFlags, FlagNUW);
----------------
This doesn't look correct?  You probably want something like `Step->getValue()->getValue().isNonNegative()`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9895
@@ +9894,3 @@
+  const SCEV *Expr = getSCEV(V);
+  const auto *AR = static_cast<const SCEVAddRecExpr *>(Expr);
+
----------------
Please use `cast<>`

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2007
@@ +2006,3 @@
+                                         Instruction *IP) {
+  const auto *A = static_cast<const SCEVAddRecExpr *>(Pred->getExpr());
+  auto *BoolType = IntegerType::get(IP->getContext(), 1);
----------------
Please use `cast<>`

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2009
@@ +2008,3 @@
+  auto *BoolType = IntegerType::get(IP->getContext(), 1);
+  Value *Check = ConstantInt::getNullValue(BoolType);
+
----------------
I think you can use `ConstantInt::getFalse(IP->getContext())` here.


http://reviews.llvm.org/D15412