[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! Please find my comments inline.

Michael


================
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
----------------
Makes sense. But then we'll need to clarify, that another assumption is that the unknown part is always `LHS` (while `RHS` is always a constant?). Maybe it's worth enforcing these assumptions with some assertions too.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:240
@@ +239,3 @@
+    SmallVector<SCEVEqualPredicate, 16> IdPreds;
+
+  public:
----------------
Hmm.. I don't think it'll work..

So, `IdPreds` is a vector of objects, and `Preds` is a vector of pointers to those objects, right? Are you going to keep different types of predicates in `Preds`? If so, where the actual objects will be stored? Will you create a separate `IdPreds`-like array for any new type of objects that can be stored in `Preds` (because you can't store objects of a type other than `SCEVEqualPredicate` in `IdPreds`, right?) ?

I think we'll have to heap allocate the objects to make the scheme versatile. For an example of how similar problem is solved, you could take a look at `ScalarEvolution::getConstant` and `UniqueSCEVs` member of class `ScalarEvolution`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9066
@@ +9065,3 @@
+
+class SCEVPredicateRewriter : public SCEVRewriteVisitor<SCEVPredicateRewriter> {
+public:
----------------
Thanks! FWIW, I like that patch, but I'd like to leave it up to Andy or Sanjoy for a final approval.


http://reviews.llvm.org/D12905