[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Junmo,

It looks better now, couple of comments/questions inline.

Michael


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1865-1870
@@ -1864,8 +1864,8 @@
     ICmpInst::Predicate Pred;
-    Instruction *LHS, *RHS;
+    Value *LHSV, *RHSV;
     BasicBlock *TrueBB, *FalseBB;
 
-    if (!match(BB->getTerminator(),
-               m_Br(m_ICmp(Pred, m_Instruction(LHS), m_Instruction(RHS)),
-                    TrueBB, FalseBB)))
+    if (!match(
+            BB->getTerminator(),
+            m_Br(m_ICmp(Pred, m_Value(LHSV), m_Value(RHSV)), TrueBB, FalseBB)))
       continue;
----------------
Is there any point from changing `LHS` and `RHS` from `Instruction` to `Value`? Looks like later on we cast it to `Instruction` anyway..

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1901-1902
@@ +1900,4 @@
+            SE.DT.dominates(EntInst, At) &&
+            (SE.LI.getLoopFor(EntInst->getParent()) == nullptr ||
+             SE.LI.getLoopFor(EntInst->getParent())->contains(At))) {
+          return Ent;
----------------
Why do we need these conditions? Isn't dominance sufficient?

Also, would it make sense to factor out this code block to a separate function (and use it both here and in `expand`)?


http://reviews.llvm.org/D15559