<div dir="ltr"><div><div><font face="monospace"><div><div> A better way to illustrate forked diamond</div><div><br></div><div>  //       H</div><div>  //      / \</div><div>  //     /   \</div><div>  //   BB    Pred</div><div>  //   / \  /|</div><div>  //   |  X  |</div><div>  //   | / \ |</div><div>  //   S1   S2</div><div>  //    \   /</div><div>  //      M</div></div><div><br></div></font></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 27, 2016 at 6:47 PM, David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">davidxl added a comment.<br>
<br>
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.<br>
<br>
Please add a test case also.<br>
<br>
<br>
================<br>
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:639<br>
@@ +638,3 @@<br>
+  //     /   \<br>
+  //   BB    Pred<br>
+  //   / \  /   \<br>
----------------<br>
Nit: can you make the art work like the following to not split S2 into two 'blocks':<br>
<br>
//<br>
  //      Head (Or Entry, Top)<br>
  //      / \<br>
  //     /   \<br>
  //   BB    Pred<br>
  //   / \   /  |<br>
  //   |  S1  |<br>
  //    \      /<br>
  //      S2<br>
  //<br>
<br>
<br>
================<br>
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:643<br>
@@ +642,3 @@<br>
+  //<br>
+  // The current block is BB and edge BB->S1 is now being evaluated.<br>
+  // As above S->BB was already selected because<br>
----------------<br>
Nit: can you make the art work like the following to not split S2 into two 'blocks':<br>
<br>
//<br>
  //      Head (Or Entry, Top)<br>
  //      / \<br>
  //     /   \<br>
  //   BB    Pred<br>
  //   / \   /  |<br>
  //   |  S1  |<br>
  //    \      /<br>
  //      S2<br>
  //<br>
<br>
<br>
================<br>
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:660<br>
@@ +659,3 @@<br>
+  // Non-topo-order cost:<br>
+  // In the worst case, S2 will not get layed out after Pred.<br>
+  // non-topo-cost = 2 * freq(S->Pred) + freq(BB->S2).<br>
----------------<br>
layed out --> laid out<br>
<br>
================<br>
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:661<br>
@@ +660,3 @@<br>
+  // In the worst case, S2 will not get layed out after Pred.<br>
+  // non-topo-cost = 2 * freq(S->Pred) + freq(BB->S2).<br>
+  // To be conservative, we can assume that min(freq(Pred->S1), freq(Pred->S2))<br>
----------------<br>
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<br>
<br>
freq(S->BB) + max(freq(Pred->S1), freq(Pred->S2).   (1)<br>
<br>
For non-top case, the saving is:<br>
<br>
freq(S->BB) + freq(BB->S1) + freq(Pred->S2)          (2)<br>
<br>
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)<br>
<br>
================<br>
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:679<br>
@@ +678,3 @@<br>
+    // Do backward checking.<br>
+    // For case 2 above, we need a backward checking to filter out edges that<br>
+    // are not 'strongly' biased. With profile data available, the check is<br>
----------------<br>
For case 1 and 2<br>
<br>
================<br>
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:683<br>
@@ -661,1 +682,3 @@<br>
+    // than two successors.<br>
+    // For case 3 above, this test is essential, even with profiling data.<br>
     // BB  Pred<br>
----------------<br>
This comment does not fit here. With profile data, such check won't be skipped, but just does not need to be as biased.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D22892" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22892</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div>