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

[John Brawn via Phabricator via llvm-commits]

john.brawn added a comment.

In https://reviews.llvm.org/D52827#1257485, @reames wrote:

> Starting with only high level design comments....
>
> I think this patch is going about things in slightly the wrong way.  As far as I can tell, there are two key components to this: 1) hoisting PHIs w/loop invariant selectors and operands, and 2) hoisting instructions from below conditional branches into conditional blocks inserted in preheader.  I think these two can and should be separate into distinct patches.
>
> I'm concerned about (2).  This really seems like a form of loop peeling, and a currently unrestricted one without a cost model.  I'm not sure I'm convinced this belongs in LICM at all.
>
> (1) could be formulated w/o (2).  The easiest way would be to form a select chain using the LIV conditions and LIV operands.  This also works for any instruction for which we have a predicated form.


I'd not considered going direct from phi to select when hoisting. It seems like it will restrict the kinds of phis that can be hoisted, but we're probably not hoisting them anyway due to the various TODOs here. I'll look into it.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:442
+// we make note of this. When we then come to hoist an instruction that's
+// conditional on such a branch we duplicate the branch and the relevant control
+// flow, then hoist the instruction into the block corresponding to its original
----------------
reames wrote:
> Basic question on design here.  Once we've hoisted the branch on the loop invariant condition, shouldn't we be able to remove the condition within the loop?  Or is this more analogous to loop peeling where we leave a copy of the control flow within the loop?
Only if we also hoist everything from the if and else blocks in the loop. Currently I'm leaving it up to simplifycfg to figur out if that's the case and clean it up if so.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:485
+    // eventually converge to a single block.
+    BasicBlock *TrueDest = BI->getSuccessor(0);
+    BasicBlock *FalseDest = BI->getSuccessor(1);
----------------
reames wrote:
> I don't think the approach you're using here works.  Consider the following:
> A -> B, C
> B-> D, E
> C-> D, F
> 
> B & C share a common successor, but it's not a unique successor.  As such, there are paths that don't converge in D.  
I'm pretty sure that only matters if E uses values defined in B (or F uses values defined in C) which are then hoisted to conditional blocks in the preheader. But in that case we'd see that the uses aren't dominated and rehoist. But this does cause problems if we insert phis instead of rehoisting, so I do need to look at what I'm doing here some more.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:798
+  // when we hoist an instruction we hoist its operands.
+  Instruction *HoistPoint = OriginalPreheader->getTerminator();
+  for (Instruction *I : HoistedInstructions) {
----------------
reames wrote:
> I think the "re-hoisting" here (which is really re-sinking right?) is probably a non-starter.  We should instead prevent hoisting.
Current behaviour of LICM is to hoist everything to the same block, and what this re-hoisting does is essentially get us that behaviour in those cases where hoisting to a conditional block doesn't work. Preventing hoisting would therefore prevent hoisting of things that are currently hoisted. We _could_ do something like delaying the decision of where to hoist things until we know if everything will end up being dominated, but re-hoisting seems much easier.


Repository:
  rL LLVM

https://reviews.llvm.org/D52827