[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.

Thanks for the reviews! Replies inline.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:470-473
@@ -460,6 +469,6 @@
 public:
   LoopAccessInfo(Loop *L, ScalarEvolution *SE, const DataLayout &DL,
                  const TargetLibraryInfo *TLI, AliasAnalysis *AA,
                  DominatorTree *DT, LoopInfo *LI,
-                 const ValueToValueMap &Strides);
+                 const ValueToValueMap &Strides, const unsigned SCEVComplexity);
 
----------------
anemet wrote:
> I am not sure I understand this interface (it is certainly under-documented and so is getInfo):
> 
> What is the point of passing strides and SCEV complexity?  Don't we know already that for each symbolic stride in Strides we will need exactly one check?  I understand that this will change when we generate predicates for other SCEV assumption, but:
> 
> Wouldn't it be better to pass a PredicateSet here initialized with the strides?
My understanding is that the strides in the Strides map can be replaced with 1 if needed. However it is not guaranteed that we will need to replace these strides with 1.

I was thinking that maybe it would be a good idea to have some VersioningParameters struct to hold all the parameters that tell us if we can use versioning? So at the moment it would be  the strides map and the SCEV complexity. Do you think that would make sense?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:239
@@ +238,3 @@
+    /// Storage for different predicates that make up this union of predicates.
+    SmallVector<SCEVEqualPredicate, 16> IdPreds;
+
----------------
sanjoy wrote:
> I agree with @mzolotukhin -- this won't work once you have different types of predicates.  You'll need a "host" to contain the allocations, perhaps using a `BumpPtrAllocator` or something similar.
Ok, this makes sense. I'll do the changes.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:240
@@ +239,3 @@
+    SmallVector<SCEVEqualPredicate, 16> IdPreds;
+
+  public:
----------------
mzolotukhin wrote:
> Hmm.. I don't think it'll work..
> 
> So, `IdPreds` is a vector of objects, and `Preds` is a vector of pointers to those objects, right? Are you going to keep different types of predicates in `Preds`? If so, where the actual objects will be stored? Will you create a separate `IdPreds`-like array for any new type of objects that can be stored in `Preds` (because you can't store objects of a type other than `SCEVEqualPredicate` in `IdPreds`, right?) ?
> 
> I think we'll have to heap allocate the objects to make the scheme versatile. For an example of how similar problem is solved, you could take a look at `ScalarEvolution::getConstant` and `UniqueSCEVs` member of class `ScalarEvolution`.
I agree, the current scheme is not ideal.

Using a ScalarEvolution-like allocation scheme seems reasonable to me for everything except SCEVUnionPredicate.
But we don't to allocate that anyway so it should be fine.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:258
@@ +257,3 @@
+    /// generated we will return a pair of nullptr.
+    std::pair<Instruction *, Instruction *>
+    generateGuardCond(Instruction *Loc, ScalarEvolution *SE);
----------------
sanjoy wrote:
> Why do you need to return a pair of instructions?  Why not just return a `Value *` computing the result of the predicate?  This will also obviate the need to create a "fake or" in the implementation, and you'll just be able to return what IRBuilder gave you.
It is mostly for consistency. This is what some of the other versioning checks return - addRuntimeChecks in LoopAccessInfo and InnerLoopVectorizer::addStrideCheck (removed by this patch).

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9111
@@ +9110,3 @@
+
+  if ((Op->E0 == E0 && Op->E1 == E1) || (Op->E0 == E1 && Op->E1 == E0))
+    return true;
----------------
sanjoy wrote:
> Why not just `return (Op->E0 == E0 && Op->E1 == E1) || (Op->E0 == E1 && Op->E1 == E0)`?
In fact if we know that LHS and RHS are different (one is a SCEVUnknown and the other a SCEVConstant) we can simplify that expression.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9199
@@ +9198,3 @@
+                       [this](SCEVPredicate *I) { return this->contains(I); });
+  return std::any_of(Preds.begin(), Preds.end(),
+                     [N](SCEVPredicate *I) { return I->contains(N); });
----------------
sanjoy wrote:
> I think this should be `std::all_of` to be consistent with the interpretation of `SCEVUnionPredicate` in `generateCheck` -- to prove `(X|Y)->Z` you need to prove `(X->Z)&&(Y->Z)`.
The union predicate function is an "and", so I think this makes the current implementation correct?


http://reviews.llvm.org/D12905