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

[Adam Nemet via llvm-commits]

anemet added a comment.

Hi Silviu,

Thanks for splitting this out, this is a much easier read now!

Adam


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:295
@@ -292,1 +294,3 @@
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVPredicateSet &Pred;
 };
----------------
I think I've commented on this too: this should be plural or have set in the name.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:545-576
@@ -539,28 +544,34 @@
 
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  std::unique_ptr<SCEVPredicateSet> Pred;
+
 private:
   /// \brief Analyze the loop.  Substitute symbolic strides using Strides.
   void analyzeLoop(const ValueToValueMap &Strides);
 
   /// \brief Check if the structure of the loop allows it to be analyzed by this
   /// pass.
   bool canAnalyzeLoop();
 
   void emitAnalysis(LoopAccessReport &Message);
 
   /// We need to check that all of the pointers in this list are disjoint
   /// at runtime.
   RuntimePointerChecking PtrRtChecking;
 
   /// \brief the Memory Dependence Checker which can determine the
   /// loop-independent and loop-carried dependences between memory accesses.
   MemoryDepChecker DepChecker;
 
   Loop *TheLoop;
   ScalarEvolution *SE;
   const DataLayout &DL;
   const TargetLibraryInfo *TLI;
   AliasAnalysis *AA;
   DominatorTree *DT;
   LoopInfo *LI;
 
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVPredicateSet ScPredicates;
+
   unsigned NumLoads;
----------------
I think I've already asked this before: why is the thing with unique_ptr needed?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:176
@@ -173,1 +175,3 @@
 
+  enum SCEVPredicateTypes { pSet, pEqual };
+
----------------
I think the enum tags are uppercase.  Also this should probably be inside the class.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:190-196
@@ +189,9 @@
+    unsigned short getType() const { return SCEVPredicateType; }
+    /// Returns true if the predicate is always true. This means that no
+    /// assumptions were made and nothing needs to be checked at run-time.
+    virtual bool isAlwaysTrue() const = 0;
+    /// Return true if we consider this to be always false or if we've
+    /// given up on this set of assumptions (for example because of the
+    /// high cost of checking at run-time).
+    virtual bool isAlwaysFalse() const = 0;
+    /// Returns true if this predicate implies \p N.
----------------
Do these really ever happen aside for reaching the check threshold?

I think that we should just have an API to query the number of checks.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1186-1189
@@ -1071,1 +1185,6 @@
 
+    /// Re-writes the SCEV according to the Predicates in \p Preds, by
+    /// applying overflow assumptions and sinking sext/zext expressions.
+    const SCEV *rewriteUsingPredicate(const SCEV *Scev, const Loop *L,
+                                      SCEVPredicateSet &A);
+
----------------
Stale comment.  I also think that the function name should be plural.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:115-119
@@ -111,1 +114,7 @@
 
+static cl::opt<unsigned>
+SCEVCheckThreshold("force-max-scev-checks", cl::init(16),
+                   cl::Hidden,
+                   cl::desc("Don't create SCEV predicates with more than "
+                            "this number of assumptions."));
+
----------------
I don't think that this should be central threshold.  It should be up to the client transformation to decide if the benefit of the transformation outweighs the overhead of the necessary checks.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:8902-8909
@@ +8901,10 @@
+
+static Instruction *getFirstInst(Instruction *FirstInst, Value *V,
+                                 Instruction *Loc) {
+  if (FirstInst)
+    return FirstInst;
+  if (Instruction *I = dyn_cast<Instruction>(V))
+    return I->getParent() == Loc->getParent() ? I : nullptr;
+  return nullptr;
+}
+
----------------
Hmm, I think we're duplicating this now into the third file.  Any better idea?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:8911
@@ +8910,3 @@
+
+struct SCEVPredicateRewriter
+    : public SCEVVisitor<SCEVPredicateRewriter, const SCEV *> {
----------------
Should probably be a class since not all members are public.

Also it needs a comment.

This may be a silly question but why we need to override all these members?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9090
@@ +9089,3 @@
+
+  IRBuilder<> OFBuilder(Loc);
+  Instruction *FirstInst = nullptr;
----------------
OF for overflow? ;)

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:318-321
@@ -317,2 +317,6 @@
 
+  // Adds code to check the overflow assumptions made by SCEV
+  std::pair<Instruction *, Instruction *>
+  addRuntimeOverflowChecks(Instruction *Loc);
+
   /// Create an empty loop, based on the loop ranges of the old loop.
----------------
Overflow again.


http://reviews.llvm.org/D12905