[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter
Michael Zolotukhin via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 15 11:57:03 PST 2016
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
More information about the llvm-commits
mailing list