[PATCH] D12905: [SCEV][LV] Introduce SCEV Predicates and use them to re-implement stride versioning

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Overall, I think this is looking much better.  I have few minor comments inline.  The only design issue (according to me) is that (I've said this inline) we're creating a fake instruction solely to satisfy an internal interface.  I think that's a code smell, but I can live with it if that's hard to fix and others are okay with having it.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:194
@@ +193,3 @@
+
+    unsigned short getType() const { return SCEVPredicateType; }
+
----------------
This is optional to fix, but I'd prefer renaming this to `getKind`, since `getType` in LLVM has the connotation of returning a `Type *`.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:213
@@ +212,3 @@
+    /// In case no checks are needed nullptr is returned.
+    virtual Value *generateCheck(Instruction *Loc, ScalarEvolution &SE,
+                                 const DataLayout &DL,
----------------
Please document `Loc`.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:216
@@ +215,3 @@
+                                 SCEVExpander &Exp) const = 0;
+    /// \brief Returns the SCEV to which this predicate applies, or nullptr
+    /// if this is a SCEVUnionPredicate.
----------------
Newline here?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:267
@@ +266,3 @@
+
+    /// \brief returns the left hand side of the equality.
+    const SCEVUnknown *getLHS() const { return LHS; }
----------------
Here, and in `getLHS`, please start the sentence with uppercase.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:285
@@ +284,3 @@
+  private:
+    typedef std::map<const SCEV *, SmallVector<const SCEVPredicate *, 4>>
+        PredicateMap;
----------------
Why not `DenseMap`?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:312
@@ +311,3 @@
+    /// which apply to \p Expr.
+    SmallVectorImpl<const SCEVPredicate *> *
+    getPredicatesForExpr(const SCEV *Expr);
----------------
I'd prefer returning an `ArrayRef<const SCEVPredicate *>`, and returning an empty `ArrayRef` if there are no predicates for `Expr`.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1271
@@ -1098,2 +1270,3 @@
     FoldingSet<SCEV> UniqueSCEVs;
+    FoldingSet<SCEVPredicate> UniquePreds;
     BumpPtrAllocator SCEVAllocator;
----------------
Please construct this in the move constructor.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:113
@@ +112,3 @@
+    const auto *CT = static_cast<const SCEVConstant *>(
+        SE->getConstant(StrideVal->getType(), 1, true));
+
----------------
There is now a `ScalarEvolution::getOne`, can you use that here?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9247
@@ +9246,3 @@
+    return FirstInst;
+  if (auto I = dyn_cast<Instruction>(V))
+    return I->getParent() == Loc->getParent() ? I : nullptr;
----------------
Use `auto *I` here.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9247
@@ +9246,3 @@
+    return FirstInst;
+  if (auto I = dyn_cast<Instruction>(V))
+    return I->getParent() == Loc->getParent() ? I : nullptr;
----------------
sanjoy wrote:
> Use `auto *I` here.
Isn't `V` always an instruction?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9258
@@ +9257,3 @@
+  ID.AddPointer(LHS);
+  ID.AddPointer(RHS);
+  void *IP = nullptr;
----------------
Don't you need to add the type of the predicate to `ID`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9283
@@ +9282,3 @@
+      for (auto *Pred : *ExprPreds)
+        if (const SCEVEqualPredicate *IPred =
+                dyn_cast<const SCEVEqualPredicate>(Pred)) {
----------------
Use `const auto *` because the type is obvious from the RHS.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9301
@@ +9300,3 @@
+
+//// SCEV predicates
+SCEVPredicate::SCEVPredicate(const FoldingSetNodeIDRef ID, unsigned short Type)
----------------
Use three slashes.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9360
@@ +9359,3 @@
+
+  // IRBuilder might fold the checks to constant expressions. Make sure we have
+  // at least one instruction by performing an OR with False.
----------------
If the `IRBuilder` returned a constant `Check`, why do you need to generate the check?  I'd either

 - change this to `assert(isa<Instruction>(Check))`, and fix the cases where the assert fails to return `true` from `isAlwaysTrue` (i.e. if the IRBuilder can prove the check is redundant, SCEV should be able to as well)

 - change the interface to allow returning something that says "always true" / "always false" in addition to returning a pair of instructions

I think creating a completely redundant instruction to satisfy internal interfaces is a code smell, and we should try to avoid it if reasonable.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9385
@@ +9384,3 @@
+  // Loop over all checks in this set.
+  for (auto Pred : Preds) {
+    if (Pred->isAlwaysTrue())
----------------
In that case, shouldn't you have

```
AllCheck = CheckBuilder.CreateAnd(AllCheck, CheckResult);
```

in `SCEVUnionPredicate::generateCheck`?  Or are you checking something different there?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9394
@@ +9393,3 @@
+    else
+      AllCheck = CheckBuilder.CreateOr(AllCheck, CheckResult);
+  }
----------------
Shouldn't this be `CreateAnd`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9394
@@ +9393,3 @@
+    else
+      AllCheck = CheckBuilder.CreateOr(AllCheck, CheckResult);
+  }
----------------
sanjoy wrote:
> Shouldn't this be `CreateAnd`?
As mentioned earlier, IIUC this should be a `CreateAnd`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9437
@@ +9436,3 @@
+  auto &Predicates = SCEVToPreds[Key];
+  Predicates.push_back(N);
+
----------------
Why not just `SCEVToPreds[Key].push_back(N);`?


http://reviews.llvm.org/D12905