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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
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
----------------
john.brawn wrote:
> 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.
Right, it makes sense when uses are Phis. Maybe it should be explicitly stated in the comment. :)


================
Comment at: lib/Transforms/Scalar/LICM.cpp:805
+      I->moveBefore(HoistPoint);
+      HoistPoint = I;
+    }
----------------
john.brawn wrote:
> 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.
But if we process `%a` and then `%b` and insert each of them before the terminator, we don't have this problem. It seems that using a vector instead of a list will spare you of this problem.


Repository:
  rL LLVM

https://reviews.llvm.org/D52827