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

[Adam Nemet via llvm-commits]

anemet added a comment.

There is a huge number of inline comments from earlier revisions that are still popping up in the new version.  This makes it pretty hard to read the patch.  Can you please check if marking those "done" would make them disappear?


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:295
@@ +294,3 @@
+
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVUnionPredicate &Preds;
----------------
Please expand this comment to explain how they are used for dep-checking.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:337-340
@@ -331,5 +336,6 @@
 
   /// Insert a pointer and calculate the start and end SCEVs.
   void insert(Loop *Lp, Value *Ptr, bool WritePtr, unsigned DepSetId,
-              unsigned ASId, const ValueToValueMap &Strides);
+              unsigned ASId, const ValueToValueMap &Strides,
+              SCEVUnionPredicate &Preds);
 
----------------
Explain the Preds parameter

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:546-547
@@ -539,1 +545,4 @@
 
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVUnionPredicate Preds;
+
----------------
Same here, please expand comment to explain how they are used.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:594-603
@@ -584,11 +593,12 @@
 
 ///\brief Return the SCEV corresponding to a pointer with the symbolic stride
 ///replaced with constant one.
 ///
 /// If \p OrigPtr is not null, use it to look up the stride value instead of \p
 /// Ptr.  \p PtrToStride provides the mapping between the pointer value and its
 /// stride as collected by LoopVectorizationLegality::collectStridedAccess.
 const SCEV *replaceSymbolicStrideSCEV(ScalarEvolution *SE,
                                       const ValueToValueMap &PtrToStride,
-                                      Value *Ptr, Value *OrigPtr = nullptr);
+                                      SCEVUnionPredicate &Preds, Value *Ptr,
+                                      Value *OrigPtr = nullptr);
 
----------------
Comment the Preds parameter.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:605-608
@@ -594,6 +604,6 @@
 
 /// \brief Check the stride of the pointer and ensure that it does not wrap in
 /// the address space.
 int isStridedPtr(ScalarEvolution *SE, Value *Ptr, const Loop *Lp,
-                 const ValueToValueMap &StridesMap);
+                 const ValueToValueMap &StridesMap, SCEVUnionPredicate &Preds);
 
----------------
Here too.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:635-636
@@ -624,3 +634,4 @@
   /// the analysis.
-  const LoopAccessInfo &getInfo(Loop *L, const ValueToValueMap &Strides);
+  const LoopAccessInfo &getInfo(Loop *L, const ValueToValueMap &Strides,
+                                const unsigned MaxSCEVPredicates = 0);
 
----------------
Didn't you want to get rid of MaxSCEVPredicates?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1783-1784
@@ -1768,3 +1782,4 @@
 const LoopAccessInfo &
-LoopAccessAnalysis::getInfo(Loop *L, const ValueToValueMap &Strides) {
+LoopAccessAnalysis::getInfo(Loop *L, const ValueToValueMap &Strides,
+                            const unsigned MaxSCEVPredicates) {
   auto &LAI = LoopAccessInfoMap[L];
----------------
Ah, MaxSCEVPredicates is unused.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:401-402
@@ -397,4 +400,4 @@
   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);
+
+  void emitSCEVChecks(Loop *L, BasicBlock *Bypass);
   /// Emit bypass checks to check any memory assumptions we may have made.
----------------
Comment missing

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:510-511
@@ -506,1 +509,4 @@
+
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVUnionPredicate &Preds;
 };
----------------
Here too please expand the comment to explain what these assumptions are used for.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:778-779
@@ -768,1 +777,4 @@
 
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVUnionPredicate &Preds;
+
----------------
Here too.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1351
@@ +1350,3 @@
+
+  unsigned SCEVCheckThreshold;
+  /// The SCEV predicate containing all the SCEV-related assumptions.
----------------
Missing comment.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1692-1701
@@ -1668,6 +1691,12 @@
 
+    unsigned SCEVThreshold = VectorizeSCEVCheckThreshold;
+    if (Hints.getForce() == LoopVectorizeHints::FK_Enabled)
+      SCEVThreshold = PragmaVectorizeSCEVCheckThreshold;
+
+    SCEVUnionPredicate Preds;
+
     // Check if it is legal to vectorize the loop.
     LoopVectorizationRequirements Requirements;
     LoopVectorizationLegality LVL(L, SE, DT, TLI, AA, F, TTI, LAA,
-                                  &Requirements, &Hints);
+                                  &Requirements, &Hints, SCEVThreshold, Preds);
     if (!LVL.canVectorize()) {
----------------
Any reason this whole logic can't be pushed into LVLegality?


http://reviews.llvm.org/D12905