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

[silviu.baranga@arm.com via llvm-commits]

sbaranga added a comment.

Thanks, James! Replies inline.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:181
@@ -180,1 +180,3 @@
+  MemoryDepChecker(ScalarEvolution *Se, const Loop *L,
+                   SCEVUnionPredicate &Preds)
       : SE(Se), InnermostLoop(L), AccessIdx(0),
----------------
jmolloy wrote:
> It doesn't make sense to me that this is plural when it doesn't relate to a set/list/vector of items. It's just one predicate, that happens to be a union.
> 
> In fact, why is this a unionpredicate anyway? Why can't it just be the superclass?
There are two ways to see it: as a collection of predicates or as one predicate. If we are adding more predicates to it maybe it makes sense to call it Preds?

We need this to be a union predicate because we're using the union predicate specific methods (that is we're adding new predicates to it, which we can only with the union predicates). I don't expect there would aver be a need to have something different then a union predicate there.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:209
@@ +208,3 @@
+    /// \brief Prints a textual representation of this predicate.
+    virtual void print(raw_ostream &OS, unsigned Depth) const = 0;
+
----------------
jmolloy wrote:
> What does Depth mean here? does it make sense to have  a default parameter for people who "just want" to print something?
Depth is for indentation. Having the Depth be 0 by default makes sense to me.


http://reviews.llvm.org/D13595