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

Thanks, Michael! Answers inline.

- Silviu


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:240
@@ +239,3 @@
+
+  public:
+    SCEVPredicateSet();
----------------
mzolotukhin wrote:
> I still don't understand the purpose of having `IdPreds`. Why can't `Preds` contain different types of predicates?
I'm basically using IdPreds for storage. It's not ideal. Would there be some other way of providing storage for different types of predicates while avoiding heap allocation?

Preds would hold a list of references and would be the thing that the algorithms use.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:247-248
@@ +246,4 @@
+
+    /// Vector with references to all predicates in this set.
+    SmallVector<SCEVPredicate *, 16> Preds;
+
----------------
mzolotukhin wrote:
> Why do we need it as a `public` member?
I needed to get access to Preds from the rewriter. Maybe having some interface for this would be better.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9068
@@ +9067,3 @@
+
+struct SCEVPredicateRewriter
+    : public SCEVVisitor<SCEVPredicateRewriter, const SCEV *> {
----------------
mzolotukhin wrote:
> Yes, you're right, I misread the code.
> 
> I think it would make sense to factor out common ('default' implementation) part into a separate class. It will also remove some code duplication from `SCEVApplyRewriter` and `SCEVParameterRewriter`. That should be a separate NFC patch, but I think we need to do it before landing this one.
Makes sense. I've create another review for this change: http://reviews.llvm.org/D13242

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9235
@@ +9234,3 @@
+  if (!Check)
+    return std::make_pair(nullptr, nullptr);
+
----------------
mzolotukhin wrote:
> Why are this and 9224 lines different?
One of these should be removed, yes.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9237-9238
@@ +9236,4 @@
+
+  Instruction *CheckInst =
+      BinaryOperator::CreateOr(Check, ConstantInt::getFalse(M->getContext()));
+  GuardBuilder.Insert(CheckInst, "scev.check");
----------------
mzolotukhin wrote:
> From previous iteration: Why do we need to do `OR` with `False`?
I wrote this code some time ago but if I remember correctly this was the same reason why LoopAccessInfo::addRuntimeChecks does an AND with True:

"We have to do this trickery because the IRBuilder might fold the check to a constant expression in which case there is no Instruction anchored in a the block.".


http://reviews.llvm.org/D12905