[PATCH] D52827: [LICM] Make LICM able to hoist phis

[John Brawn via Phabricator via llvm-commits]

john.brawn marked 6 inline comments as done.
john.brawn added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:470
+    BasicBlock *OriginalPreheader = CurLoop->getLoopPreheader();
+    OriginalPreheaderChildren = DT->getNode(OriginalPreheader)->getChildren();
+  }
----------------
mkazantsev wrote:
> Preheader by definition has only one successor which is the loop header. Therefore, `OriginalPreheaderChildren` always has one element which is header's DTNode. So it doesn't look like you even need this field, just take loop header (it doesn't change, right?)
> 
> Or am I missing something here?
> 
> And btw, if it needs to be a field and a vector, use `SmallVector` instead of `std::vector`.
Yes, I think you're right. I'll do some testing to double-check then change this.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:672
+  // re-hoisted if they end up not dominating all of their uses.
+  std::list<Instruction *> HoistedInstructions;
+
----------------
mkazantsev wrote:
> Why not `SmallVector` and `push_back`? That wouldn't make any difference in terms of effect but less memory-consuming.
The iteration order when rehoisting will have to be changed (because we have to visit instructions before the instructions they use, i.e. later instructions before earlier instructions), but yes that will work.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:793
 
+  // If we hoisted instructions to a conditional block they may not dominate
+  // their uses that weren't hoisted. If so make them unconditional by moving
----------------
mkazantsev wrote:
> This is the part I don't understand. If the hoist destination doesn't dominate instruction's users then it also doesn't dominate the original instruction. Is LICM able to hoist to such locations? Is there an example of that?
We get this when we hoist an instruction but don't hoist all of its uses. An easy example of this is a phi where one operand is loop invariant and the other is not. The hoisted loop invariant operand will not dominate its use. In the tests I've added @conditional_use, @rehoist, and @diamond_with_extra_in_edge are all examples of where rehoisting happens.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:805
+      I->moveBefore(HoistPoint);
+      HoistPoint = I;
+    }
----------------
mkazantsev wrote:
> Do we really need to change the hoist point? Why not just hoist of them before preheader's terminator? That would make this code simpler.
If we have something like
```
%a = add i32 %loop_invariant, 1
%b = mul i32 %a, 2
```
then if %b does not dominate its uses we have to rehoist %b, at which point %a no longer dominates %b and has to be rehoisted to before %a. That's what HoistPoint is doing, it's making sure we rehoist instructions before rehoisted instructions that use them.


Repository:
  rL LLVM

https://reviews.llvm.org/D52827