[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Junmo Park via llvm-commits]

flyingforyou marked 6 inline comments as done.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1834
@@ +1833,3 @@
+  if (SE.isSCEVable(I->getType()) && SE.getSCEV(I) == S)
+    return I;
+
----------------
sanjoy wrote:
> I'm worried that we're possibly doing too much work here (`getSCEV` can be expensive).  Can we further filter on the type of `I`?  For instance, if the width of `I->getType()` and `S->getType()` are different, then we don't need to bother calling `getSCEV` on `I`.
> 
> Edit:  I'm much less worried about doing too much work here, after seeing the guard on `SE.hasOperand` in the caller; but I still think filtering on the type is a good idea.
Thanks for good sugesstion.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1870
@@ +1869,3 @@
+      if (SE.hasOperand(SE.getSCEV(LHS), S)) {
+        LHS = FindSameSCEVInst(SE, S, LHS, 0);
+        if (LHS != nullptr && SE.DT.dominates(LHS, At))
----------------
sanjoy wrote:
> Please don't reuse the `LHS` variable here.  Perhaps `auto *ExistingInst = ..`?
Thanks for guiding this. As you said, using lambda could be more clear than before. Great!.


http://reviews.llvm.org/D15559