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

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

sbaranga added a comment.

Hi, Adam, Hal,

Thanks for the reviews! I've replied inline. I should have a new version shortly.

Silviu


================
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;
----------------
anemet wrote:
> I think I've already asked this before: why is the thing with unique_ptr needed?
I'm sure I had some reason at some point of making it a unique_ptr, but I can't remember. I think it should be possible to remove the unique_ptr part.

================
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.
----------------
anemet wrote:
> 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.
isAlwaysFalse would currently be true only when reaching the threshold.
Having an API to return the number of checks (or maybe something that estimates the cost of the checks?), would be more clear.

================
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."));
+
----------------
anemet wrote:
> 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.
Ok, I'll remove this.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:8911
@@ +8910,3 @@
+
+struct SCEVPredicateRewriter
+    : public SCEVVisitor<SCEVPredicateRewriter, const SCEV *> {
----------------
anemet wrote:
> 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?
I think because SCEVVisitor is a template that uses the visit* methods without defining them, users must define all the methods (or get a compilation error). Not really nice.

I'll make the changes (add a comment and convert to a class).

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9157
@@ +9156,3 @@
+
+void SCEVPredicateSet::add(const SCEVPredicate *N) {
+  if (Preds.size() > SCEVCheckThreshold || N->isAlwaysFalse()) {
----------------
hfinkel wrote:
> Can this take a threshold override, or similar, as a parameter to override SCEVCheckThreshold? We had specifically decided that loops decorated with #pragma clang vectorize(enable), which asks for vectorization but does not assert safety, would generate as many checks as necessary to enable vectorization (or be bound by some very large limit). For this case, we'll need to override the limit (or, at least, have a much larger limit). Generically, I'm skeptical of embedding the limit in SCEV at all; I think that the caller should always provide an appropriate limit for whatever happens to be its use case.
> 
That makes sense to me. There is currently the override in SCEV, but from your argument it looks like it should be passed in from the user.

Would that put a complexity bound on our rewriting algorithms / estimates for how expensive using these predicates can be? So far I've worked under the assumption that we would have a limited number of predicates.


http://reviews.llvm.org/D12905