[PATCH] D20505: Codegen: Outline for chains of tail-duplicable blocks.

[Eric Christopher via llvm-commits]

echristo added a comment.

Did a quick run through for clarity. A few inline comments. Few requests to break things up. Check for coding style nits across the entire set of code and feel free to run clang format on the lines you've changed.

Thanks for the work so far!

-eric


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:724
@@ -646,2 +723,3 @@
     const BlockFilterSet *BlockFilter = nullptr) {
+
   BlockChain &Chain = *BlockToChain[MBB];
----------------
Sadly I'm not sure if you're adding or deleting whitespace here. Either way feel free to do it separately.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:756-757
@@ -680,1 +755,4 @@
+    BlockFilterSet *BlockFilter) {
+  assert(BB && "BB must not be null.\n");
+  assert(BlockToChain[BB] == &Chain && "BlockToChainMap mis-match.\n");
   MachineFunction &F = *BB->getParent();
----------------
Go ahead and commit this separately (along with the one below).

Also "mismatch" and you shouldn't need the \n.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:825-826
@@ +824,4 @@
+        }
+        bool TailBlockRemoved = false;
+        auto RemovalCallback =
+            [&](MachineBasicBlock *RemBB) {
----------------
Can you document everything that's going on here more please? In particular, what's going on with the callback here and why it needs to be a callback rather than happening on the spot.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1288
@@ +1287,3 @@
+
+/// \brief Finds unavoidable blocks within a loop.
+///
----------------
Once again I can't remember if we have autobrief turned on or not...

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1297
@@ +1296,3 @@
+  // Find the nearest common dominator of all of F's terminators.
+  MachineBasicBlock * Terminator = nullptr;
+  for (MachineBasicBlock *MBB : Exits) {
----------------
Formatting.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1312
@@ +1311,3 @@
+  for (MachineBasicBlock *MBB : L.getBlocks()) {
+    if (MDT->dominates(MBB, Terminator)) {
+      UnavoidableBlocks.insert(MBB);
----------------
Coding style nit: no braces around single lines.


http://reviews.llvm.org/D20505