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

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Silviu,

Thanks for the changes, the code looks much better now! Please find some remarks inline.

Michael


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:194-196
@@ +193,5 @@
+    virtual void print(raw_ostream &OS, unsigned Depth) const = 0;
+    /// Generates a run-time check for this predicate.
+    virtual Value *generateCheck(Instruction *Loc, ScalarEvolution *SE,
+                                 const DataLayout *DL, SCEVExpander &Exp) = 0;
+  };
----------------
It might return `nullptr` in certain cases - please document them.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:201-202
@@ +200,4 @@
+  /// SCEVEqualPredicate - This class represents an assumption that two SCEV
+  /// expressions are equal, and this can be checked at run-time. We assume#
+  /// that the right hand side is a SCEVUnknown.
+  ///
----------------
s/assume#/assume/

By the way, why is the assumption (that RHS is SCEVUnknown) needed?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:240
@@ +239,3 @@
+
+  public:
+    SCEVPredicateSet();
----------------
I still don't understand the purpose of having `IdPreds`. Why can't `Preds` contain different types of predicates?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:247-248
@@ +246,4 @@
+
+    /// Vector with references to all predicates in this set.
+    SmallVector<SCEVPredicate *, 16> Preds;
+
----------------
Why do we need it as a `public` member?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:433
@@ +432,3 @@
+                 MemoryDepChecker::DepCandidates &DA,
+                 unsigned SCEVCheckThreshold, SCEVPredicateSet &Preds)
+      : DL(Dl), AST(*AA), LI(LI), DepCands(DA), IsRTCheckAnalysisNeeded(false),
----------------
Nitpick: here the predicate set is called `Preds`, in other functions (e.g. `RuntimePointerChecking::insert`) it's called `Pred`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9068
@@ +9067,3 @@
+
+struct SCEVPredicateRewriter
+    : public SCEVVisitor<SCEVPredicateRewriter, const SCEV *> {
----------------
Yes, you're right, I misread the code.

I think it would make sense to factor out common ('default' implementation) part into a separate class. It will also remove some code duplication from `SCEVApplyRewriter` and `SCEVParameterRewriter`. That should be a separate NFC patch, but I think we need to do it before landing this one.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9202
@@ +9201,3 @@
+}
+
+void SCEVEqualPredicate::print(raw_ostream &OS, unsigned Depth) const {
----------------
sbaranga wrote:
> The convention that I used so far was to generate the negative test (ICmpNE). I think the loop vectorizer / loop access analysis were doing the same thing for their checks? Would keeping it like this be a problem? 
I think it's fine, but at least we should explicitly state somewhere that we're doing it this way. It'll prevent future readers of this code from being caught by surprise.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9235
@@ +9234,3 @@
+  if (!Check)
+    return std::make_pair(nullptr, nullptr);
+
----------------
Why are this and 9224 lines different?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9237-9238
@@ +9236,4 @@
+
+  Instruction *CheckInst =
+      BinaryOperator::CreateOr(Check, ConstantInt::getFalse(M->getContext()));
+  GuardBuilder.Insert(CheckInst, "scev.check");
----------------
From previous iteration: Why do we need to do `OR` with `False`?


http://reviews.llvm.org/D12905