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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1023
@@ +1022,3 @@
+        BlockChain::iterator ChainEnd = Chain.end();
+        BB = *(--ChainEnd);
+        continue;
----------------
BB = *std::prev(Chain.end())

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1834
@@ +1833,3 @@
+                          Removed, DuplicatedToBB);
+  if (Removed) {
+    // Do 2 things: If we duplicated into BB, we need to update
----------------
early return if not removed to reduce code nesting level

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1835
@@ +1834,3 @@
+  if (Removed) {
+    // Do 2 things: If we duplicated into BB, we need to update
+    // unscheduled predecessors. Then try to duplicate again.
----------------
remove 'into'

================
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
----------------
std::prev

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1844
@@ +1843,3 @@
+      // not the whole chain.
+      markBlockSuccessors(Chain, DupBB, LoopHeaderBB, BlockFilter);
+      // Now try to duplicate again.
----------------
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.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1849
@@ +1848,3 @@
+      DupPred = *(--ChainEnd);
+      maybeTailDuplicateBlock(DupBB, DupPred, Chain, BlockFilter,
+                              PrevUnplacedBlockIt,
----------------
Can you give more explanation on the iterative tailDup? 

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1885
@@ +1884,3 @@
+  if ((IsSimple || BB->succ_size() != 1) &&
+      TailDup.shouldTailDuplicate(*F, IsSimple, *BB)) {
+    // Update UnscheduledPredecessors to reflect tail-duplication.
----------------
Should an early return be done here?
if (!IsSimple && BB->succ_size() == 1) return false;
if (!TailDup.shouldTailDuplicate(...) return false;

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1890
@@ +1889,3 @@
+      BlockChain* PredChain = BlockToChain[Pred];
+      if (Pred == LPred || (BlockFilter && !BlockFilter->count(Pred))
+          || PredChain == &Chain)
----------------
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)

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1893
@@ +1892,3 @@
+        continue;
+      if (TailDup.canTailDuplicate(BB, Pred)) {
+        for (MachineBasicBlock *NewSucc : BB->successors()) {
----------------
Need to count the number of other Pred that BB can tail duplicate into.

If that number is zero, should return false.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1905
@@ +1904,3 @@
+                  << getBlockName(NewSucc) << "\n");
+            BlockToChain[NewSucc]->UnscheduledPredecessors++;
+          }
----------------
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.

================
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
----------------
Early return false when '!CanDupToChain'

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1934
@@ +1933,3 @@
+            RemoveList = EHPadWorkList;
+          for (auto it = RemoveList.begin(); it != RemoveList.end(); ++it) {
+            if (*it == RemBB) {
----------------
It is cleaner to use RemoveList.erase(remove_if(...)..) pattern

Also bypass if RemBB's unscheduled pred number is not zero.


https://reviews.llvm.org/D18226