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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1013
@@ +1012,3 @@
+        BlockChain::iterator ChainEnd = Chain.end();
+        BB = *std::prev(ChainEnd);
+        continue;
----------------
The update of BB should be wrapped into 'repeatedlyTailDuplicateBlock'.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1826
@@ +1825,3 @@
+  // If BB was duplicated into LPred, it is now scheduled, but because it was
+  // removed markChainSuccessors won't be called for its chain. Instead we call
+  // markBlockSuccessors for LPred to achieve the same effect.
----------------
Missing coma. ".. it was removed, markChainSuccessors .."

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1836
@@ +1835,3 @@
+    MachineBasicBlock *DupBB, *DupPred;
+    BlockChain::iterator ChainEnd = Chain.end();
+    DupBB = *(--ChainEnd);
----------------
Add a comment here that Chain End is updated when DupBB is removed by the removal call back.

There also seem to be a bug here: if TailDup happens, but DupBB is not removed, the loop will check the same BB again. Should it just break the loop when removed is not true?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1863
@@ +1862,3 @@
+///                        only be true if Removed is also true.
+void MachineBlockPlacement::maybeTailDuplicateBlock(
+    MachineBasicBlock *BB, MachineBasicBlock *LPred,
----------------
It is cleaner to make this interface return bool (either Removed or DuplicatedToLPred)


https://reviews.llvm.org/D18226