[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Junmo Park via llvm-commits]

flyingforyou marked an inline comment as done.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1841
@@ +1840,3 @@
+
+  SCEVSearch Search(Base);
+  visitAll(S, Search);
----------------
mzolotukhin wrote:
> Shouldn't it be `SCEVTraversal<SCEVSearch>`?
I think current code is ok.
But there is same code in ScalarEvolution class. So I remove this function. Thanks.

================
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;
+  }
----------------
mzolotukhin wrote:
> This loop will always end on the first operand.
> Should it be
> ```
> if (Op)
>   return Op;
> ```
> ?
Oh...Thanks.

================
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))
----------------
mzolotukhin wrote:
> 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.
> Also, I wonder if we should check for the case where LHS is a ConstantInt and RHS is the instruction.
> 


I am not sure. Isn't it optional??

================
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());
----------------
mzolotukhin wrote:
> 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.
Yes. You're right.

================
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(),
----------------
mzolotukhin wrote:
> 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).
I think this is another issue about `findExistingExpansion` . I am not sure that I can fix the issue in this patch. 

================
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.
+
----------------
mzolotukhin wrote:
> 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?
It will not be unroll factor. I just want to make example that trip count can be divided by original codes. Not belong to IV's step is unknown likes first example. The main point of first example(test) is that if IV's step is unknown, we should make division for getting tripcount for unrolled loop.

> we don't need to generate a new expression for the new tripcount. Is this correct?
This is correct. 

If you have better explanation about this example, please let me know. 


================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:53
@@ -27,1 +52,2 @@
+
 !0 = !{i64 1, i64 100}
----------------
mzolotukhin wrote:
> Nitpick: this metadata is probably unnecessary.
This is original code. I'm not sure it's ok to remove.
Sanjoy, Can I remove this?


http://reviews.llvm.org/D15559