[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 Michael,

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

> Hi Silviu,
>
> Thanks, I see the general direction now. I found several spots that need some clean-up (you could find them inline), but apart from that I have a question - currently `PredicateSet` only works as `OR` of several checks. Do we need (or might we need in future) sets of predicates, combined with `AND`? I tend to think that we'll need both. I suspect that supporting them both might add non-trivial amount of work to this patch, so we can keep it for later, but I think that as a default option we need `AND`, not `OR`. What do you think?


The current implementation of PredicateSet is actually an AND. The checks do (or (not p)), for p in the set. So we're effectively getting the condition that would be used to invalidate the predicates. I don't see a clear case for 'OR' at this moment but I guess it might be required at some point.

Thanks,
Silviu


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:245
@@ +244,3 @@
+    //SmallVector<SCEVAddRecOverflowPredicate, 16> AddRecOverflows;
+    SmallVector<SCEVEqualPredicate, 16> IdPreds;
+
----------------
mzolotukhin wrote:
> What is `IdPreds` and how is it different from `Preds`?
Right now it's not different, but I was thinking that in the future it could contain different types of predicates.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:255-256
@@ +254,4 @@
+    generateGuardCond(Instruction *Loc, ScalarEvolution *SE);
+
+    /// Implementation of the SCEVPredicate interface
+    bool isAlwaysTrue() const override;
----------------
It does, yes! This should be consistent with the interface used by the loop vectorizer / loop access analysis to add runtime checks. I'll add documentation for this in the next version.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:8911
@@ +8910,3 @@
+
+struct SCEVPredicateRewriter
+    : public SCEVVisitor<SCEVPredicateRewriter, const SCEV *> {
----------------
mzolotukhin wrote:
> 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`.
> 
> 
I think we have a different case here, as we would like to visit SCEV expressions. I think that the LoopUnrollPass examples visits instructions? 

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9202
@@ +9201,3 @@
+}
+
+void SCEVEqualPredicate::print(raw_ostream &OS, unsigned Depth) const {
----------------
The convention that I used so far was to generate the negative test (ICmpNE). I think the loop vectorizer / loop access analysis were doing the same thing for their checks? Would keeping it like this be a problem? 

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9298
@@ +9297,2 @@
+  llvm_unreachable("Unknown SCEV predicate type!");
+}
----------------
Makes sense. Thanks!


http://reviews.llvm.org/D12905