[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Michael Zolotukhin via llvm-commits]

mzolotukhin requested changes to this revision.
mzolotukhin added a reviewer: mzolotukhin.
This revision now requires changes to proceed.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1841
@@ +1840,3 @@
+
+  SCEVSearch Search(Base);
+  visitAll(S, Search);
----------------
Shouldn't it be `SCEVTraversal<SCEVSearch>`?

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1846-1847
@@ +1845,4 @@
+
+// Recursive search of the IR level operands of LHS and RHS to see if any of
+// them are congruent to S. This function is called by findExistingExpansion.
+static Instruction *FindSameSCEVInst(ScalarEvolution &SE, const SCEV *S,
----------------
What are `LHS` and `RHS` here?

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1858-1860
@@ +1857,5 @@
+  for (unsigned i = 0; i < I->getNumOperands(); ++i) {
+    Instruction *Op = FindSameSCEVInst(
+        SE, S, dyn_cast<Instruction>(I->getOperand(i)), ++depth);
+    return Op;
+  }
----------------
This loop will always end on the first operand.
Should it be
```
if (Op)
  return Op;
```
?

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1885-1886
@@ -1842,6 +1884,4 @@
 
-    if (SE.getSCEV(LHS) == S && SE.DT.dominates(LHS, At))
-      return LHS;
-
-    if (SE.getSCEV(RHS) == S && SE.DT.dominates(RHS, At))
-      return RHS;
+    if (isa<Instruction>(RHSV)) {
+      RHS = cast<Instruction>(RHSV);
+      if (SE.getSCEV(LHS) == S && SE.DT.dominates(LHS, At))
----------------
You could do
```
if (RHS = dyn_cast<Instruction>(RHSV)) {
...
```

Also, I wonder if we should check for the case where LHS is a ConstantInt and RHS is the instruction.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:318-319
@@ +317,4 @@
+  // prolog code and one for a new loop preheader
+  BasicBlock *PEnd = SplitEdge(PH, Header, DT, LI);
+  BasicBlock *NewPH = SplitBlock(PEnd, PEnd->getTerminator(), DT, LI);
+  BranchInst *PreHeaderBR = cast<BranchInst>(PH->getTerminator());
----------------
Could we avoid changing anything unless we're confident we're going to unroll? E.g. we can split `PH`--->`Header` edge, then split `PEnd` basic block, and then we can decide to bail out.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:346-347
@@ -343,4 +345,4 @@
   //  extra iterations = run-time trip count % (loop unroll factor + 1)
   Value *TripCount = Expander.expandCodeFor(TripCountSC, TripCountSC->getType(),
                                             PreHeaderBR);
   Value *BECount = Expander.expandCodeFor(BECountSC, BECountSC->getType(),
----------------
Please make sure that expander also knows how to reuse existing values. From my last investigation of PR24920 I remember that fixing `findExistingExpansion` alone isn't enough, as we generate expensive trip count anyway, ignoring the fact that we can reuse existing value for it.

Also, you might take a look at my patch for fixing this bug, which I posted in D12765 (see 6th comment from the end).

================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:27-28
@@ -26,1 +26,4 @@
 
+;; We expect this loop to be unrolled, because IV's step is minus one.
+;; In this case, we don't need to generate dvision for computing trip count.
+
----------------
I think this description is a bit misleading. We can unroll not because IV's step is minus one, but because tripcount is known to be divisible by `%conv7` (which will be unroll factor, right?), and we don't need to generate a new expression for the new tripcount. Is this correct?

================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:53
@@ -27,1 +52,2 @@
+
 !0 = !{i64 1, i64 100}
----------------
Nitpick: this metadata is probably unnecessary.


http://reviews.llvm.org/D15559