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

[James Molloy via llvm-commits]

jmolloy added a comment.

Hi Silviu,

It looks like Michael and Sanjoy have done most of the heavy lifting on this review, so for me I think it's in a good shape by this point.

I have a bunch of comments though.

James


================
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),
----------------
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?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:53
@@ -52,2 +52,3 @@
   class SCEVAddRecExpr;
+  class SCEVConstant;
   class SCEV;
----------------
The sorting seems off here.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:174
@@ -168,1 +173,3 @@
 
+  //===--------------------------------------------------------------------===//
+  /// SCEVPredicate - This class represents an assumption made using SCEV
----------------
Is there a need for this separator line? I can't see any prior art anywhere else in the file

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:186
@@ +185,3 @@
+  protected:
+    unsigned short SCEVPredicateType;
+
----------------
Why is this an unsigned short and not an enum SCEVPredicateTypes?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:189
@@ +188,3 @@
+  public:
+    enum SCEVPredicateTypes { PSET, PEQUAL };
+
----------------
These don't match the usual LLVM style of enum naming. Normally we use a prefix, underscore then an UpperCamelCase identifier:

    P_Set, P_Equal

================
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;
+
----------------
What does Depth mean here? does it make sense to have  a default parameter for people who "just want" to print something?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:211
@@ +210,3 @@
+
+    /// \brief Generates a run-time check for this predicate. All checking
+    /// instructions will be inserted before \p Loc. The result is a pair of
----------------
If you're going to use \brief, keep the first sentence separate from the rest of the text with an empty line.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:216
@@ +215,3 @@
+    /// the first and last instructions that were generated for this check.
+    /// The last instruction produces the check result.
+    virtual std::pair<Value *, Value *>
----------------
Is it guaranteed that the values are contiguous and that no other instrucitons are interleaved between them? (i assume not, best to make this explicit).

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:267
@@ +266,3 @@
+    const SCEV *getExpr() const;
+
+    std::pair<Value *, Value *> generateCheck(Instruction *Loc,
----------------
Remove unneeded blank line.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:325
@@ +324,3 @@
+    const SCEV *getExpr() const override;
+
+    std::pair<Value *, Value *> generateCheck(Instruction *Loc,
----------------
Unneeded blank line.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:337
@@ +336,3 @@
+    static inline bool classof(const SCEVPredicate *P) {
+      return P->getKind() == PSET;
+    }
----------------
Surely PUNION would be more appropriate?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:130
@@ -129,3 +135,2 @@
   const SCEV *Ex = SE->getBackedgeTakenCount(Lp);
-
   const SCEV *ScStart = AR->getStart();
----------------
Removed useful newline

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9376
@@ +9375,3 @@
+
+  return std::make_pair(static_cast<Instruction *>(First),
+                        static_cast<Instruction *>(Last));
----------------
You can just do:

   return {static_cast..., static_cast...};

Yay C++11!

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9403
@@ +9402,3 @@
+
+    if (!AllCheck)
+      AllCheck = Check;
----------------
Either all braces or no braces - don't mix braces in if/else statements.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9411
@@ +9410,3 @@
+
+  return std::make_pair(FirstInst, AllCheck);
+}
----------------
Can use C++11 syntax here too.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9418
@@ +9417,3 @@
+        Set->Preds.begin(), Set->Preds.end(),
+        [this](const SCEVPredicate *I) { return this->implies(I); });
+
----------------
You could probably use std::bind here.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9426
@@ +9425,3 @@
+  return std::any_of(SCEVPreds.begin(), SCEVPreds.end(),
+                     [N](const SCEVPredicate *I) { return I->implies(N); });
+}
----------------
and here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:401
@@ -397,3 +400,3 @@
   void emitVectorLoopEnteredCheck(Loop *L, BasicBlock *Bypass);
-  /// Emit bypass checks to check if strides we've assumed to be one really are.
-  void emitStrideChecks(Loop *L, BasicBlock *Bypass);
+  /// Emit a bypasss check to see if all of the SCEV assumptions we've
+  /// had to make are correct.
----------------
bypassssssss


http://reviews.llvm.org/D13595