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

In http://reviews.llvm.org/D12905#252980, @mzolotukhin wrote:

> 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


Hi Michael,

This won't work on any new cases of StrideVersioning, it should be equivalent to the existing implementation.
The main motivation is to introduce the SCEV predicates. I plan to add predicates for overflow tests (see http://reviews.llvm.org/D10161 for the overall direction).

I've found that range checking predicates could also be useful in some cases (the use case I have for this would be canonicalization of "a <= b" into "a < b + 1" in SCEV), but I would have to think more if this is actually needed.

Thanks,
Silviu


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:574-575
@@ -565,1 +573,4 @@
 
+  /// The SCEV predicate containing all the SCEV-related assumptions.
+  SCEVPredicateSet ScPredicates;
+
----------------
mzolotukhin wrote:
> Where is this used?
This is an artefact of rebasing and should be removed.

================
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.
----------------
mzolotukhin wrote:
> 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)?
I'm not opposed to changing the names. SCEVPredicateBase/SCEVUnionPredicate could work.

================
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.
----------------
mzolotukhin wrote:
> Should it be called `Invalid` or something like this then?
"Invalid" would be better, yes.


http://reviews.llvm.org/D12905