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

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Silviu,

Thanks for doing this, I think this could be a nice improvement. As for now, several questions: does it work on any new cases, compared to the original StrideVersioning implementation? Do you plan to add any other types of predicates in future?

Also, some comments inline.

Thanks,
Michael


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:574-575
@@ -565,1 +573,4 @@
 
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVPredicateSet ScPredicates;
+
----------------
Where is this used?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:207
@@ +206,3 @@
+
+  class SCEVEqualPredicate : public SCEVPredicate {
+    // We assume that E0 == E1
----------------
Please add some description for this class.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:239-240
@@ +238,4 @@
+  ///
+  class SCEVPredicateSet : public SCEVPredicate {
+  protected:
+    /// Flag used to track if this predicate set is invalid.
----------------
The names `SCEVPredicate` and `SCEVPredicateSet` are a bit confusing: usually, one doesn't assume that a `SomethingSet` is derived from `Something`. Would it make sense to rename them to something like `SCEVPredicateBase`/`SCEVUnionPredicate` (not insisting on these particular names)?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:241-242
@@ +240,4 @@
+  protected:
+    /// Flag used to track if this predicate set is invalid.
+    bool Never;
+    /// Storage for different predicates that make up this Predicate Set.
----------------
Should it be called `Invalid` or something like this then?

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:109-115
@@ -106,10 +108,9 @@
     // Replace symbolic stride by one.
-    Value *One = ConstantInt::get(StrideVal->getType(), 1);
+    Value *VOne = ConstantInt::get(StrideVal->getType(), 1);
+    ConstantInt *One = static_cast<ConstantInt *>(VOne);
     ValueToValueMap RewriteMap;
     RewriteMap[StrideVal] = One;
 
-    const SCEV *ByOne =
-        SCEVParameterRewriter::rewrite(OrigSCEV, *SE, RewriteMap, true);
-    DEBUG(dbgs() << "LAA: Replacing SCEV: " << *OrigSCEV << " by: " << *ByOne
-                 << "\n");
-    return ByOne;
+    SCEVPredicateSet P = Pred;
+    const SCEVEqualPredicate SI(SE->getSCEV(StrideVal), SE->getConstant(One));
+    P.add(&SI);
----------------
Instead of storing `VOne` and `One` in a separate variables, I think we can use `ScalarEvolution::getConstant(Type *Ty, uint64_t V, bool isSigned)` here.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:8911
@@ +8910,3 @@
+
+struct SCEVPredicateRewriter
+    : public SCEVVisitor<SCEVPredicateRewriter, const SCEV *> {
----------------
anemet wrote:
> sbaranga wrote:
> > 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).
> > 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.
> 
> Can this be derived from SCEVParameterRewriter since we only support the equality predicate right now?  Then we can extend it later as we add the other predicates.
> 
> users must define all the methods (or get a compilation error). Not really nice.
No, you don't have to override all of them, you only need to define the ones you want to change. For generic case, you could override `visitInstruction` method. As an example, you could look at `class UnrolledInstAnalyzer` in `lib/Transforms/Scalar/LoopUnrollPass.cpp`.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:9158
@@ +9157,3 @@
+void SCEVPredicateSet::add(const SCEVPredicate *N) {
+  if (Preds.size() > SCEVCheckThreshold || N->isAlwaysFalse()) {
+    Never = true;
----------------
I think it should be `>=` since we haven't added the new predicate yet.


http://reviews.llvm.org/D12905