<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 24, 2017 at 5:19 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>
<br>
<br>
================<br>
<span class="">Comment at: lib/CodeGen/<wbr>MachineBlockPlacement.cpp:1086<br>
+  if (DupCandidates.size() != 0)<br>
+    std::make_heap(DupCandidates.<wbr>begin(), DupCandidates.end());<br>
+  while (DupCandidates.size() != 0) {<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> Why not just stable sort it? The vector should be of size 1 for most of the cases.  Also why do you need position ?<br>
</span>Will just sort the vector.<br>
<br>
Position is because we rely on the successor order being stable and the first successor being a subtle hint. Without the position, we lose track of whether the block in the vector came before or after the block we picked without tail duplication.<br>
<span class=""><br></span></blockquote><div><br></div><div>would stable_sort not work for this case?</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
================<br>
Comment at: lib/CodeGen/<wbr>MachineBlockPlacement.cpp:1098<br>
+    if (<wbr>canTailDuplicateUnplacedPreds(<wbr>BB, Succ, Chain, BlockFilter)<br>
+        && isProfitableToTailDup(BB, Succ, AdjustedSumProb, Chain,<br>
+                                     BlockFilter)) {<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> isProfitableToTailDup assumes the baseline layout does not pick Succ.  The assumption may not be true here as there are other two possibilities:<br>
> 1) Succ == BestSucc.BB in the base layout<br>
> 2) BestSucc.BB == null in the base layout (all BB's successors have conflicts).<br>
><br>
> In such two cases, isProfitable check should probably be skipped (as it is benefitial)<br>
</span>1.<br>
Succ won't equal BestSucc.BB because of the continue. These blocks were not chosen by the first loop by construction.<br>
<br>
2.<br>
Good catch. I'll add that.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D28583" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D28583</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>