[PATCH] D11687: Make cost estimation in RewriteLoopExitValues smarter

[Igor Laevsky via llvm-commits]

igor-laevsky added a comment.

Thanks for the comments!


================
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);
----------------
sanjoy wrote:
> I'd call this `getExistingSCEVExpansion`, making it clear that we're looking for a value that already exists.
Renamed to `tryToGetExistingExpansion`.  I'd like to leave `tryTo` in the name to emphasise that function can sometimes fail to find any value.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1894
@@ -1853,3 +1893,3 @@
     // compute.
     BasicBlock *ExitingBB = L->getExitingBlock();
     if (!ExitingBB)
----------------
sanjoy wrote:
> 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?).
In `TryToGetValueForSCEVAt` we are looking for:
`  ExitCond == S, where ExitCond is LHS or RHS`
On success we return ExitCond, because it is existing LLVM IR value for S.

For division we are looking for:
`  (ExitCond - 1) == S, where ExitCond is LHS or RHS`
If we move this subtraction logic into `TryToGetValueForSCEVAt` we will not be able to return anything since we actually didn't find any pre-existing LLVM IR value.

I suspect we can completely remove division special handling if:
  1. `(ExitCond - 1) == S <=> ExitCond == (S + 1)` (or other non AddRec expression with S)
  2. `isHighCostExpansionHelper` can recursively look into expressions. I.e `IsHighCost(a + b) == IsHighCost(a) || isHighCost(b)` (this is true right now)
  3. We will update `TryToGetValueForSCEVAt` to look into exiting blocks

However I am afraid to break something, so I will do it in separate change. (But first I will check that division case is covered by existing tests.)


Repository:
  rL LLVM

http://reviews.llvm.org/D11687