[PATCH] D12905: [SCEV][LV] Introduce SCEV Predicates and use them to re-implement stride versioning

[silviu.baranga@arm.com via llvm-commits]

sbaranga marked 80 inline comments as done.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9243-9250
@@ +9242,10 @@
+
+static Instruction *getFirstInst(Instruction *FirstInst, Value *V,
+                                 Instruction *Loc) {
+  if (FirstInst)
+    return FirstInst;
+  if (auto I = dyn_cast<Instruction>(V))
+    return I->getParent() == Loc->getParent() ? I : nullptr;
+  return nullptr;
+}
+
----------------
We remove this from the loop vectorizer so we end up with the same number of copies. There's still some code duplication.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9360
@@ +9359,3 @@
+
+  // IRBuilder might fold the checks to constant expressions. Make sure we have
+  // at least one instruction by performing an OR with False.
----------------
sanjoy wrote:
> If the `IRBuilder` returned a constant `Check`, why do you need to generate the check?  I'd either
> 
>  - change this to `assert(isa<Instruction>(Check))`, and fix the cases where the assert fails to return `true` from `isAlwaysTrue` (i.e. if the IRBuilder can prove the check is redundant, SCEV should be able to as well)
> 
>  - change the interface to allow returning something that says "always true" / "always false" in addition to returning a pair of instructions
> 
> I think creating a completely redundant instruction to satisfy internal interfaces is a code smell, and we should try to avoid it if reasonable.
Yes, I think you're right. I would go with the first option.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9385
@@ +9384,3 @@
+  // Loop over all checks in this set.
+  for (auto Pred : Preds) {
+    if (Pred->isAlwaysTrue())
----------------
sanjoy wrote:
> In that case, shouldn't you have
> 
> ```
> AllCheck = CheckBuilder.CreateAnd(AllCheck, CheckResult);
> ```
> 
> in `SCEVUnionPredicate::generateCheck`?  Or are you checking something different there?
See the comment bellow.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9394
@@ +9393,3 @@
+    else
+      AllCheck = CheckBuilder.CreateOr(AllCheck, CheckResult);
+  }
----------------
sanjoy wrote:
> sanjoy wrote:
> > Shouldn't this be `CreateAnd`?
> As mentioned earlier, IIUC this should be a `CreateAnd`.
The generated code checks for the negated condtion. So we basically end up with
(or (not predicate1), (not predicate2), ...) which is the same as
(not (and prediacte1, predicate2, ..)

We do this because the current users already use this interface.
This also avoids the need to explicitly create negations (we just create the negated check for each predicate).

I think this at least needs a comment. Do you have a strong preference about this?


http://reviews.llvm.org/D12905