[PATCH] D22892: Codegen: MachineBlockPlacement Improve probability layout.

[David Li via llvm-commits]

davidxl added a comment.

This is a good change. I actually wanted to get rid of the forward check which is always true for case 2 and redundant for case 1. Now it is also bad for case 3.

Please add a test case also.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:639
@@ +638,3 @@
+  //     /   \
+  //   BB    Pred
+  //   / \  /   \
----------------
Nit: can you make the art work like the following to not split S2 into two 'blocks':

//
  //      Head (Or Entry, Top)
  //      / \
  //     /   \
  //   BB    Pred
  //   / \   /  |
  //   |  S1  |
  //    \      /
  //      S2   
  //


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:643
@@ +642,3 @@
+  //
+  // The current block is BB and edge BB->S1 is now being evaluated.
+  // As above S->BB was already selected because
----------------
Nit: can you make the art work like the following to not split S2 into two 'blocks':

//
  //      Head (Or Entry, Top)
  //      / \
  //     /   \
  //   BB    Pred
  //   / \   /  |
  //   |  S1  |
  //    \      /
  //      S2   
  //


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:660
@@ +659,3 @@
+  // Non-topo-order cost:
+  // In the worst case, S2 will not get layed out after Pred.
+  // non-topo-cost = 2 * freq(S->Pred) + freq(BB->S2).
----------------
layed out --> laid out

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:661
@@ +660,3 @@
+  // In the worst case, S2 will not get layed out after Pred.
+  // non-topo-cost = 2 * freq(S->Pred) + freq(BB->S2).
+  // To be conservative, we can assume that min(freq(Pred->S1), freq(Pred->S2))
----------------
Another way to explain in terms of savings instead of cost: the savings is the total freq of the fall through edges. In topo case, the savings is 

freq(S->BB) + max(freq(Pred->S1), freq(Pred->S2).   (1)

For non-top case, the saving is:

freq(S->BB) + freq(BB->S1) + freq(Pred->S2)          (2)

When freq(Pred->S2) > freq(Pred->S1), (2) is strictly larger than (1). In the opposite case, the check below will also lead to (2) > (1)

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:679
@@ +678,3 @@
+    // Do backward checking.
+    // For case 2 above, we need a backward checking to filter out edges that
+    // are not 'strongly' biased. With profile data available, the check is
----------------
For case 1 and 2

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:683
@@ -661,1 +682,3 @@
+    // than two successors.
+    // For case 3 above, this test is essential, even with profiling data.
     // BB  Pred
----------------
This comment does not fit here. With profile data, such check won't be skipped, but just does not need to be as biased.


Repository:
  rL LLVM

https://reviews.llvm.org/D22892