[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

This is looking close to ready to land now.  Some nitpicky comments inline.


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1833
@@ +1832,3 @@
+
+  if (SE.isSCEVable(I->getType()) && SE.getSCEV(I) == S)
+    return I;
----------------
Minor:  I'd add a small comment that you're doing the pre-check on using `isSCEVable` to avoid creating too many `SCEVUnknown` expressions.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1827
@@ +1826,3 @@
+// S. This function is called by findExistingExpansion.
+static Instruction *FindSameSCEVInst(ScalarEvolution &SE, const SCEV *S,
+                                     Instruction *I, unsigned Depth) {
----------------
How about renaming this to `FindCongruentInst`, or `FindInstWithSameSCEV`. `FindSameSCEVInst` does not sound okay grammatically.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1834
@@ +1833,3 @@
+  if (SE.isSCEVable(I->getType()) && SE.getSCEV(I) == S)
+    return I;
+
----------------
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.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1865
@@ -1842,3 +1864,3 @@
 
-    if (SE.getSCEV(LHS) == S && SE.DT.dominates(LHS, At))
-      return LHS;
+    if (Instruction *LHS = dyn_cast<Instruction>(LHSV)) {
+      if (SE.getSCEV(LHS) == S && SE.DT.dominates(LHS, At))
----------------
These two identical blocks should definitely be combined into a lambda.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1866
@@ +1865,3 @@
+    if (Instruction *LHS = dyn_cast<Instruction>(LHSV)) {
+      if (SE.getSCEV(LHS) == S && SE.DT.dominates(LHS, At))
+        return LHS;
----------------
I'd "CSE" the two calls to `getSCEV(LHS)` to avoid the extra dictionary lookup.

================
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))
----------------
Please don't reuse the `LHS` variable here.  Perhaps `auto *ExistingInst = ..`?


http://reviews.llvm.org/D15559