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

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a reviewer: sanjoy.
sanjoy added a comment.
This revision now requires changes to proceed.

Reviewed the SCEV bits.  I don't know enough about the other bits to comment on those.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:184
@@ +183,3 @@
+    unsigned short getType() const { return SCEVPredicateType; }
+    /// \brief Returns the estimated complexity of this predicate.
+    /// This is roughly measured in the number of run-time checks required.
----------------
Minor: I'd put newlines between method declarations.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:191
@@ +190,3 @@
+    /// \brief Returns true if this predicate implies \p N.
+    virtual bool contains(const SCEVPredicate *N) const = 0;
+    /// \brief Prints a textual representation of this predicate.
----------------
Also minor, but why not just call it `implies`?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:197
@@ +196,3 @@
+    /// In case no checks are needed nullptr is returned.
+    virtual Value *generateCheck(Instruction *Loc, ScalarEvolution *SE,
+                                 const DataLayout *DL, SCEVExpander &Exp) = 0;
----------------
Please document `Loc`, and pass in things that cannot be `nullptr`, like `SE` or `DL` as references.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:208
@@ +207,3 @@
+    // We assume that E0 == E1
+    const SCEV *E0;
+    const SCEV *E1;
----------------
The restriction that `LHS` is a `SCEVUnknown` seems somewhat arbitrary; but if you want to keep it, please change the type of `E0` to be `SCEVUnknown`.

Also, rename `E0` and `E1` to `LHS` and `RHS` to sync with the documentation, or change the documentation to refer to these as `E0` or `E1`.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:220
@@ +219,3 @@
+
+    const SCEV *getLHS() { return E0; }
+    const SCEV *getRHS() { return E1; }
----------------
Same comment here -- if your invariant is that `E0` is a `SCEVUnknown`, then make that obvious in the type.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:234
@@ +233,3 @@
+  /// SCEVUnionPredicate - This class represents a composition of other
+  /// SCEV predicates, and is the class that most clients will interact with.
+  ///
----------------
I haven't read the whole patch yet, but I think the doc should state if a `SCEVUnionPredicate` represents a logical and or a logical or (or sometimes one and sometimes the other) of the predicates it contains.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:239
@@ +238,3 @@
+    /// Storage for different predicates that make up this union of predicates.
+    SmallVector<SCEVEqualPredicate, 16> IdPreds;
+
----------------
I agree with @mzolotukhin -- this won't work once you have different types of predicates.  You'll need a "host" to contain the allocations, perhaps using a `BumpPtrAllocator` or something similar.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:258
@@ +257,3 @@
+    /// generated we will return a pair of nullptr.
+    std::pair<Instruction *, Instruction *>
+    generateGuardCond(Instruction *Loc, ScalarEvolution *SE);
----------------
Why do you need to return a pair of instructions?  Why not just return a `Value *` computing the result of the predicate?  This will also obviate the need to create a "fake or" in the implementation, and you'll just be able to return what IRBuilder gave you.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9106
@@ +9105,3 @@
+bool SCEVEqualPredicate::contains(const SCEVPredicate *N) const {
+  const SCEVEqualPredicate *Op = dyn_cast<const SCEVEqualPredicate>(N);
+
----------------
Use `const auto *`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9111
@@ +9110,3 @@
+
+  if ((Op->E0 == E0 && Op->E1 == E1) || (Op->E0 == E1 && Op->E1 == E0))
+    return true;
----------------
Why not just `return (Op->E0 == E0 && Op->E1 == E1) || (Op->E0 == E1 && Op->E1 == E0)`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9127
@@ +9126,3 @@
+
+  Value *C = Builder.CreateICmpNE(Expr0, Expr1, "ident.check");
+  return C;
----------------
Why not just `return Builder.CreateICmpNE(...)`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9150
@@ +9149,3 @@
+std::pair<Instruction *, Instruction *>
+SCEVUnionPredicate::generateGuardCond(Instruction *Loc, ScalarEvolution *SE) {
+  IRBuilder<> GuardBuilder(Loc);
----------------
As I've mentioned in the declaration of `generateGuardCond`, I don't see why you need to return a range of instructions.  I'd rather have this return a `Value *`, and do away with `getFirstInst` and the fake `or` instruction.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9199
@@ +9198,3 @@
+                       [this](SCEVPredicate *I) { return this->contains(I); });
+  return std::any_of(Preds.begin(), Preds.end(),
+                     [N](SCEVPredicate *I) { return I->contains(N); });
----------------
I think this should be `std::all_of` to be consistent with the interpretation of `SCEVUnionPredicate` in `generateCheck` -- to prove `(X|Y)->Z` you need to prove `(X->Z)&&(Y->Z)`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9209
@@ +9208,3 @@
+void SCEVUnionPredicate::add(const SCEVPredicate *N) {
+  if (const SCEVEqualPredicate *EP = dyn_cast<const SCEVEqualPredicate>(N)) {
+    for (auto Pred : IdPreds)
----------------
Nit: here and elsewhere, prefer using `const auto *` or `auto *` when the type is obvious from the RHS.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9210
@@ +9209,3 @@
+  if (const SCEVEqualPredicate *EP = dyn_cast<const SCEVEqualPredicate>(N)) {
+    for (auto Pred : IdPreds)
+      if (Pred.contains(EP))
----------------
[Optional] Why not `std::any_of`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9213
@@ +9212,3 @@
+        return;
+    IdPreds.push_back(*EP);
+    Preds.push_back(&IdPreds.back());
----------------
As mentioned earlier, this allocation scheme is not quite right.


http://reviews.llvm.org/D12905