[PATCH] D12905: [SCEV][LV] Introduce SCEV Predicates and use them to re-implement stride versioning
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 25 18:22:19 PDT 2015
mzolotukhin added a comment.
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?
Best regards,
Michael
================
Comment at: include/llvm/Analysis/ScalarEvolution.h:245
@@ +244,3 @@
+ //SmallVector<SCEVAddRecOverflowPredicate, 16> AddRecOverflows;
+ SmallVector<SCEVEqualPredicate, 16> IdPreds;
+
----------------
What is `IdPreds` and how is it different from `Preds`?
================
Comment at: include/llvm/Analysis/ScalarEvolution.h:254-255
@@ +253,4 @@
+
+ /// Vector with references to all predicates in this set.
+ SmallVector<SCEVPredicate *, 16> Preds;
+
----------------
This probably should be private/protected.
================
Comment at: include/llvm/Analysis/ScalarEvolution.h:260-261
@@ +259,4 @@
+ /// similar to other run-time checks used for versioning.
+ std::pair<Instruction *, Instruction *>
+ generateGuardCond(Instruction *Loc, ScalarEvolution *SE);
+
----------------
Does it return first and last instruction in the generated check? Could we document it?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9030-9031
@@ +9029,4 @@
+bool SCEVEqualPredicate::isAlwaysTrue() const {
+ if (E0 == E1) return true;
+ return false;
+}
----------------
Maybe just
`return E0 == E1;`
?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9045
@@ +9044,3 @@
+
+ Value *C = Builder.CreateICmpNE(Expr0, Expr1, "ident.check");
+ return C;
----------------
The predicate is called Equal, should we generate `ICmpEQ` for consistency here?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9061
@@ +9060,3 @@
+ IdPreds = Old.IdPreds;
+ for (unsigned II = 0; II < IdPreds.size(); ++II)
+ Preds.push_back(&IdPreds[II]);
----------------
Please avoid computing `IdPreds.size()` on every iteration. Even better, we could use range loop here.
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9069
@@ +9068,3 @@
+
+ for (auto II = Preds.begin(), EE = Preds.end(); II != EE; ++II) {
+ const SCEVPredicate *OA = *II;
----------------
Another candidate for a range loop.
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9070-9071
@@ +9069,4 @@
+ for (auto II = Preds.begin(), EE = Preds.end(); II != EE; ++II) {
+ const SCEVPredicate *OA = *II;
+ if (!OA->isAlwaysTrue())
+ return false;
----------------
What does `OA` stand for? Maybe get rid of this var at all?
Actually, this entire function could probably be replaced with `std::all_of`.
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9088
@@ +9087,3 @@
+ if (isAlwaysTrue())
+ return std::pair<Instruction *, Instruction *>(tnullptr, tnullptr);
+
----------------
Could we just return `std::make_pair(nullptr, nullptr)` as we do below?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9101-9103
@@ +9100,5 @@
+
+ Instruction *CheckInst =
+ BinaryOperator::CreateOr(Check, ConstantInt::getFalse(M->getContext()));
+ OFBuilder.Insert(CheckInst, "scev.check");
+
----------------
Why do we need to do or with `False`?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9117
@@ +9116,3 @@
+ // Loop over all checks in this set.
+ for (auto II = Preds.begin(), EE = Preds.end(); II != EE; ++II) {
+ SCEVPredicate *OA = *II;
----------------
Range loop?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9138-9148
@@ +9137,13 @@
+
+ if (const SCEVPredicateSet *Set = dyn_cast<const SCEVPredicateSet>(N)) {
+ for (auto II = Set->Preds.begin(), EE = Set->Preds.end(); II != EE; ++II) {
+ if (!contains(*II))
+ return false;
+ }
+ return true;
+ }
+ for (auto II = Preds.begin(), EE = Preds.end(); II != EE; ++II) {
+ if ((*II)->contains(N))
+ return true;
+ }
+ return false;
----------------
Good candidates for `std::all_of`/`std::any_of`.
http://reviews.llvm.org/D12905
More information about the llvm-commits
mailing list