<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 6, 2017 at 1:36 PM, Kyle Butt via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">iteratee added inline comments.<br>
<span class=""><br>
<br>
================<br>
Comment at: test/CodeGen/PowerPC/tail-dup-<wbr>break-cfg.ll:7<br>
+; The code for tail-duplication during layout will produce the layout:<br>
+; test1<br>
+; test2<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> This test demonstrates that  this patch helps when test2 is more likely (but the branch probability is not biased enough to make the successor selection to pick test2 as successor without the patch).<br>
><br>
> What happens if body1 a more likely successor such that even with the patch, body1 will be selected as test1's successor. Later when block test2 is checked, it is still beneficial to tail duplicate it. Does it happen with the patch?<br>
</span>This patch only prefers blocks that can be tail-duplicated if they are more probable.<br>
<br>
selectBestSuccessor looks at the blocks in order and picks the most probable block that doesn't have a CFG conflict, or is hot enough to ignore the CFG conflict. Most of that logic is in hasBetterLayoutPredecessor. This patch just adds "can tail-duplicate to unplaced preds" as another condition to allow CFG-breaking.<br>
<br>
If body1 is laid out after test1, then test2 will not be copied into the end of test1, as test1 has more than one successor. Since test2 would be laid out after body1, it would not be tail-duplicated anywhere.<br></blockquote><div><br></div><div><br></div><div>Right -- it is not profitable to taildup test2 in this case.</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In an if-else case where both bodies go to a small test afterward:<br>
```<br>
    t1<br>
   / \<br>
 b1   e1<br>
   \ /<br>
    t2<br>
```<br>
<br>
The existing code will duplicate the test without this patch, producing either<br>
    t1 b1 t2 ... e1(t2) or t1 e1 t2 ... b2(t2)<br>
<br>
I can create that test if you'd like, but since it's not changed by this patch, it should go in as a separate change.<br>
<br></blockquote><div><br></div><div>Ok sounds fine to me.</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="https://reviews.llvm.org/D27742" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D27742</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>