[PATCH] D11687: Make cost estimation in RewriteLoopExitValues smarter

[Sanjoy Das]

sanjoy added a comment.

This needs test cases.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:121
@@ -121,1 +120,3 @@
+    bool isHighCostExpansion(const SCEV *Expr, Loop *L,
+                             const Instruction *At=nullptr) {
       SmallPtrSet<const SCEV *, 8> Processed;
----------------
Nit: spacing around `=`.

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:121
@@ -121,1 +120,3 @@
+    bool isHighCostExpansion(const SCEV *Expr, Loop *L,
+                             const Instruction *At=nullptr) {
       SmallPtrSet<const SCEV *, 8> Processed;
----------------
sanjoy wrote:
> Nit: spacing around `=`.
Please document what `At` does here.

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:197
@@ -195,1 +196,3 @@
 
+    /// \brief Try to find llvm-ir value for 'S' avaliable at the point 'At'.
+    // 'L' is a hint which tells in which loop to look for the suitable value.
----------------
Nit: "available"

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:197
@@ -195,1 +196,3 @@
 
+    /// \brief Try to find llvm-ir value for 'S' avaliable at the point 'At'.
+    // 'L' is a hint which tells in which loop to look for the suitable value.
----------------
sanjoy wrote:
> Nit: "available"
Nit: I'd use "LLVM IR"

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:201
@@ +200,3 @@
+    // 'At'. Return nullptr if value was not found.
+    // Note that this function does not perform exaustive search. I.e if it
+    // didn't found any value it does not mean that there is no such value.
----------------
Nit: "exhaustive".

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:202
@@ +201,3 @@
+    // Note that this function does not perform exaustive search. I.e if it
+    // didn't found any value it does not mean that there is no such value.
+    Value *TryToGetValueForSCEVAt(const SCEV *S, const Instruction *At,
----------------
Nit: "didn't find"

================
Comment at: include/llvm/Analysis/ScalarEvolutionExpander.h:203
@@ +202,3 @@
+    // didn't found any value it does not mean that there is no such value.
+    Value *TryToGetValueForSCEVAt(const SCEV *S, const Instruction *At,
+                                  Loop *L);
----------------
I'd call this `getExistingSCEVExpansion`, making it clear that we're looking for a value that already exists.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1823
@@ +1822,3 @@
+  // Look for suitable value in simple conditions at the loop latches. This is
+  // based on the fact that 'S' is probably an induction variable evaluated
+  // after loop 'L'.
----------------
The way we're using this now in `RewriteLoopExitValues`, `S` is not an induction variable, but is an operand to the loop's backedge comparison which happens to be equal to the trip count of the loop.

I'd just leave in the first sentence in the comment, and delete the rest.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1894
@@ -1853,3 +1893,3 @@
     // compute.
     BasicBlock *ExitingBB = L->getExitingBlock();
     if (!ExitingBB)
----------------
Should this division case also use `TryToGetValueForSCEVAt`?  The search logic here is basically identical, except the SCEV subtraction in the end (and perhaps we should change `TryToGetValueForSCEVAt` to look at exiting blocks too?).


http://reviews.llvm.org/D11687