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

[John Brawn via Phabricator via llvm-commits]

john.brawn marked 2 inline comments as done.
john.brawn added a comment.

In https://reviews.llvm.org/D52827#1257963, @john.brawn wrote:

> 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.


After trying this out and running lnt/spec2000/spec2006 it gives no improvements (either by itself or combined with https://reviews.llvm.org/D50723). By comparison the current approach gives no improvement by itself, and 2% improvement in spec2000/254.gap combined with https://reviews.llvm.org/D50723 (on Cortex-A57).



================
Comment at: lib/Transforms/Scalar/LICM.cpp:485
+    // eventually converge to a single block.
+    BasicBlock *TrueDest = BI->getSuccessor(0);
+    BasicBlock *FalseDest = BI->getSuccessor(1);
----------------
john.brawn wrote:
> 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.
It turns out that this is taken care of by the check that BT dominates CommonSucc. I've restructured the code here a little (as in some cases we weren't picking TrueDest/FalseDest when it was a triangle destination) and adjusted the comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:546
+        assert(!BI && "BB is expected to be the target of at most one branch");
+        BI = Pair.first;
+      }
----------------
reames wrote:
> What about indirectly dependent blocks?  (e.g. D in A->B, C, B->D)
Then we don't do anything. In the case that we're hoisting a phi in D we make sure to call getHoistedBlock on B and C beforehand which takes care of duplicating the control flow.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:797
+  // so we iterate through them in first-in-last-out order which ensures that
+  // when we hoist an instruction we hoist its operands.
+  Instruction *HoistPoint = OriginalPreheader->getTerminator();
----------------
john.brawn wrote:
> efriedma wrote:
> > Would it be better to insert a PHI node, rather than re-hoist?
> I considered that, but went with this as it gives more similar results to the current behaviour in the cases where we end up not hoisting any phis. However I'm currently working on fixing the various TODOs here and it looks like maybe I will need to go for the approach of inserting phis. I'm currently working on it.
Inserting a phi, combined with relaxing the dominated check in registerPossiblyHoistableBranch, can lead to cases where we get the phi value from the 'wrong' predecessor (e.g. in the diamond_with_extra_in_edge test). It's probably not the case that this can happen without that relaxation but I'd rather not risk it.


https://reviews.llvm.org/D52827