[PATCH] D43256: [MBP] Move a latch block with conditional exit and multi predecessors to top of loop

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1758
 
-/// \brief Find the best loop top block for layout.
+// If BottomBlock has only one successor OldTop, in most cases it is profitable
+// to move it before OldTop, except following case:
----------------
BottomBlock  --> bottom block BB


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1759
+// If BottomBlock has only one successor OldTop, in most cases it is profitable
+// to move it before OldTop, except following case:
+//
----------------
the following case.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1768
+//         BB-----
+//
+// In this case we keep its original layout.
----------------
Add some arrows in the graph and explain more in text why it is not beneficial


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1792
+// the max predecessor frequency. It is the number of reduced taken branches
+// when move the latch to the top of loop.
+bool
----------------
Add more comment about the intention of this method: 

The method checks if the reduced taken branches is  less than increased taken branch (to the exit block when rotation happens). If yes, it returns true.



================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1822
+///     2. a block has two successors, one is old top, another is exit
+///        it has more than one predecessors
 ///
----------------
--> and it has more than one predecessors.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1828
+///        predecessors jump to it, then fall through to loop header. So all its
+///        predecessors except P can reduce one taken branch.
 MachineBasicBlock *
----------------
Add comment that the reduced taken branches will be compared with the increased taken branch to the loop exit block.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1864
           MBFI->printBlockFreq(dbgs(), Pred) << " freq\n");
-    if (Pred->succ_size() > 1)
+    if (Pred->succ_size() > 2)
       continue;
----------------
Why is this check needed?


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1880
+      // Move the candidate to top must reduce taken branches.
+      if (hasRarePredecessors(Pred, OutBB))
+        continue;
----------------
It makes assumption that there is an existing fall through to the exit bb. If not, it is always beneficial to rotate.


================
Comment at: test/CodeGen/X86/move_latch_to_loop_top.ll:74
+
+; More blocks can be moved before header.
+;CHECK-LABEL: test3:
----------------
perhaps draw a diagram here.


https://reviews.llvm.org/D43256