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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1893
@@ +1892,3 @@
+    return;
+  // Update UnscheduledPredecessors to reflect tail-duplication.
+  for (MachineBasicBlock *Pred : BB->predecessors()) {
----------------
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              |
   \__________|

================
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");
----------------
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

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1910
@@ +1909,3 @@
+                << getBlockName(Pred) << " -> "
+                << getBlockName(BB) << " -> "
+                << getBlockName(NewSucc) << "\n");
----------------
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


https://reviews.llvm.org/D18226