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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

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.



================
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
----------------
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?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:472
+  void registerPossiblyHoistableBranch(BranchInst *BI) {
+    // We can only hoist conditional branches with loop variant operands where
+    // both branch destinations are in the loop.
----------------
loop invariant operands


================
Comment at: lib/Transforms/Scalar/LICM.cpp:485
+    // eventually converge to a single block.
+    BasicBlock *TrueDest = BI->getSuccessor(0);
+    BasicBlock *FalseDest = BI->getSuccessor(1);
----------------
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.  


================
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;
+      }
----------------
What about indirectly dependent blocks?  (e.g. D in A->B, C, B->D)


================
Comment at: lib/Transforms/Scalar/LICM.cpp:767
+      if (PHINode *PN = dyn_cast<PHINode>(&I)) {
+        if (CurLoop->hasLoopInvariantOperands(PN) && CFH.canHoistPHI(PN)) {
+          // Redirect incoming blocks first to ensure that we create hoisted
----------------
Minor: I'd suggest sinking the invariant operand check into the helper.  Reading the helper in isolation is currently confusing.  


================
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) {
----------------
I think the "re-hoisting" here (which is really re-sinking right?) is probably a non-starter.  We should instead prevent hoisting.


Repository:
  rL LLVM

https://reviews.llvm.org/D52827