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

[Kyle Butt via llvm-commits]

iteratee added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1893
@@ +1892,3 @@
+    return;
+  // Update UnscheduledPredecessors to reflect tail-duplication.
+  for (MachineBasicBlock *Pred : BB->predecessors()) {
----------------
davidxl wrote:
> It is not correct to do.   BB and Pred are in the same nested loop that already been laid out. You can not duplicate BB into Pred here.
> 
> LPred
>   |
>   |
>  BB <-----
>   |               \
>   ..                 \
>   Pred              |
>    \__________|
I don't see why not. There's not a correctness issue, and if BB was the loop top of the inner loop, it was never considered to be duplicated. Now is the chance that we have to do it.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1905
@@ +1904,3 @@
+        BlockChain *NewChain = BlockToChain[NewSucc];
+        if (NewChain != &Chain && NewChain != PredChain) {
+          DEBUG(dbgs() << "Bumping unscheduled pred count for chain:\n");
----------------
davidxl wrote:
> I think a list of Preds that BB can be duplicated into need to be passed in.
> 
> The reason doing update here is not bullet proof is that if later if there are other heuristic to decide not to tailDup, then the early update will be wrong. It needs to be tied to the actual transformation
The goal of this loop is not to find constraints.

I agree it would be cleaner to get the list out of tail-duplication. I'll do that.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1910
@@ +1909,3 @@
+                << getBlockName(Pred) << " -> "
+                << getBlockName(BB) << " -> "
+                << getBlockName(NewSucc) << "\n");
----------------
davidxl wrote:
> There are two problems here
> 1) many Pred (other than LPred) has been skipped as illegal candidates, but tailDupiicator does not know about will happily do tailDup -- as the tailDuplicator does not know about the new constraints from MBP
> 2) if the total number of candidate predecessors that BB can tailDuped into is fewer than 2, there is no point do the tail duplication
I think you misunderstand the point of this loop. The point is not to find "legal candidates", but to find the candidates for which duplicating will require update.

Because of that, we don't have an accurate count, and don't want one. We'll gladly let tailDuplicator do its job.


https://reviews.llvm.org/D18226