[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