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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

The SCEV parts of the change is looking much better now.  I didn't read through the logic carefully this time, and I only have minor stylistic issues.  I'm happy to give a LGTM for the SCEV bits contingent on the stylistic issues getting addressed.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:267
@@ +266,3 @@
+    bool implies(const SCEVPredicate *N) const override;
+    void print(raw_ostream &OS, unsigned Depth) const override;
+    bool isAlwaysTrue() const override;
----------------
Use a default value for `Depth` here as well.

Also, very minor, you might want to override `operator<<` too, just for consistency with the rest of the codebase.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:111
@@ -110,3 +110,3 @@
 
-    const SCEV *ByOne =
-        SCEVParameterRewriter::rewrite(OrigSCEV, *SE, RewriteMap, true);
+    const auto *U = static_cast<const SCEVUnknown *>(SE->getSCEV(StrideVal));
+    const auto *CT =
----------------
Use `cast<>`

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9317
@@ +9316,3 @@
+
+static Instruction *getFirstInst(Instruction *FirstInst, Value *V,
+                                 Instruction *Loc) {
----------------
Can we name this function better?  I don't have a better suggestion though.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9322
@@ +9321,3 @@
+  if (auto *I = dyn_cast<Instruction>(V))
+    return I->getParent() == Loc->getParent() ? I : nullptr;
+  return nullptr;
----------------
When can `I->getParent()` be not equal to `Loc->getParent()`?


http://reviews.llvm.org/D13595