[PATCH] D44766: Extend peeling to help invariant motion

[Evgeny Stupachenko via Phabricator via llvm-commits]

evstupac added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:266
+        // If there is an instruction with invariant operands and unsafe to
+        // speculate - that is because of unreachable exit block prior to it.
+        // Peeling first iteration will prove the instruction is safe.
----------------
efriedma wrote:
> This heuristic is a bit suspicious.  I'm not sure we always run LICM immediately before unrolling.  And even if we do, peeling won't help in a lot of situations; a load could alias another memory operation, or it might not dominate the loop latch.
This is not only about a load. Division also counts.

>I'm not sure we always run LICM immediately before unrolling.
It is enough to run it somewhere after unroll/peel.

>a load could alias another memory operation
We should be able to hoist out Invariant loads in most cases. At least breaking a dependency is always good. And peeling should not hurt here.

>it might not dominate the loop latch
true. There should be a check on this.



================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:531
   BasicBlock *Latch = L->getLoopLatch();
-  BasicBlock *Exit = L->getUniqueExitBlock();
+  BasicBlock *Exit = L->getUniqueReachableExitBlock();
 
----------------
efriedma wrote:
> This is really hacky.
> 
> There really isn't any reason we can't peel loops with multiple exits; it just isn't implemented yet.  Please fix it properly.
Well. I agree that putting these methods to LoopAnalysis is not good.
But this is as "hacky" as previous implementation limited to loops with one exit from latch.
The reason I limit peeling to just unreachable exits case is that these exits have close to 0 frequency. For other multiexit loops it is harder to estimate profitability.

However you are right and peeling of multiexit loops is doable and (if you feel this could be profitable) I can create a separate patch for this.



Repository:
  rL LLVM

https://reviews.llvm.org/D44766