[PATCH] D18226: Codegen: Tail-duplicate during placement.

[Kyle Butt via llvm-commits]

iteratee added a comment.

Applied some suggestions, answered others.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1840
@@ +1839,3 @@
+      BlockChain::iterator ChainEnd = Chain.end();
+      DupBB = *(--ChainEnd);
+      // If we duplicated into DupBB, we need to update unscheduled
----------------
davidxl wrote:
> std::prev
Can't. I need to actually update the iterator so that I can check to see if there is an additional block in the chain.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1844
@@ +1843,3 @@
+      // not the whole chain.
+      markBlockSuccessors(Chain, DupBB, LoopHeaderBB, BlockFilter);
+      // Now try to duplicate again.
----------------
davidxl wrote:
> This does not look right the place to update. The successors of DupBB may also get more unscheduled predecessors. The update should be done in one place where the total number of Preds (BB is dupped into) is known.
You're correct, It only needs to be done once because only one block is actually going from unscheduled to scheduled: BB.
That makes the function simpler overall and easier to understand.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1890
@@ +1889,3 @@
+      BlockChain* PredChain = BlockToChain[Pred];
+      if (Pred == LPred || (BlockFilter && !BlockFilter->count(Pred))
+          || PredChain == &Chain)
----------------
davidxl wrote:
> should also skip if Pred is in the same chain as BB (e.g, when BB is a loop head of another loop, and Pred is that loop's latch)
No, even if they're in the same chain, blocks that end up with both as predecessors still have an additional predecessor.

I think you're thinking of the check that I do below of NewChain vs PredChain.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1893
@@ +1892,3 @@
+        continue;
+      if (TailDup.canTailDuplicate(BB, Pred)) {
+        for (MachineBasicBlock *NewSucc : BB->successors()) {
----------------
davidxl wrote:
> Need to count the number of other Pred that BB can tail duplicate into.
> 
> If that number is zero, should return false.
No. because although we don't have predecessors to worry about (Loop filter), there may be unfiltered predecessors that we can duplicate into, and now is the only time to do so.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1905
@@ +1904,3 @@
+                  << getBlockName(NewSucc) << "\n");
+            BlockToChain[NewSucc]->UnscheduledPredecessors++;
+          }
----------------
davidxl wrote:
> To early to do the update. Wait until the tailDup actually happens. Actually, do we really need to update? When tailDup happens, Pred will become NewSucc's new predecessor, and Pred is still not scheduled.
We can't do it any later. BB may disappear after tail duplication.

I could make tail duplication return a vector of blocks that were duplicated into, and save the successor list before duplication.
That may be easier to follow, but wouldn't be any more correct.

Yes, we need to update. For each duplication the successors now have an additional unscheduled predecessor

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1910
@@ +1909,3 @@
+    }
+    CanDupToChain = TailDup.canTailDuplicate(BB, LPred);
+    // This has to be a callback because none of it can be done after
----------------
davidxl wrote:
> Early return false when '!CanDupToChain'
That would be incorrect. There are plenty of cases where we can't duplicate into the layout predecessor where we still want to perform tail duplication.


https://reviews.llvm.org/D18226