<div dir="auto"><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Jan 25, 2017 9:27 AM, "Xinliang David Li" <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> wrote:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div class="quoted-text">On Wed, Jan 25, 2017 at 9:18 AM, Kyle Butt <span dir="ltr"><<a href="mailto:iteratee@google.com" target="_blank">iteratee@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><span><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Jan 25, 2017 9:00 AM, "Xinliang David Li" <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>> wrote:<br type="attribution"><blockquote class="m_3530431773613264075m_-1277612103756587726quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div class="m_3530431773613264075m_-1277612103756587726quoted-text">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>Comment at: lib/CodeGen/MachineBlockPlacem<wbr>ent.cpp:1086<br>
+ if (DupCandidates.size() != 0)<br>
+ std::make_heap(DupCandidates.b<wbr>egin(), DupCandidates.end());<br>
+ while (DupCandidates.size() != 0) {<br>
----------------<br>
</span><span>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><br></span></blockquote><div><br></div></div><div>would stable_sort not work for this case?</div></div></div></div></blockquote></div></div></div><div dir="auto"><br></div></span><div dir="auto">No. We have to know if the candidate came before or after the successor chosen without duplication.</div></div></blockquote><div><br></div></div><div>Why is it important to track that ?</div></div></div></div></blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">Chandler can back me up here, but front ends and users rely on source order of branches being a slight hint in the absence of other evidence. Going through in a second pass the only way to preserve that is to somehow record the order of the first pass. Stable sort just doesn't keep enough information.</div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><font color="#888888"><div><br></div><div>David</div></font><div class="quoted-text"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><span><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="m_3530431773613264075m_-1277612103756587726quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><font color="#888888"><div><br></div><div>David</div></font><div class="m_3530431773613264075m_-1277612103756587726quoted-text"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
<br>
================<br>
Comment at: lib/CodeGen/MachineBlockPlacem<wbr>ent.cpp:1098<br>
+ if (canTailDuplicateUnplacedPreds<wbr>(BB, Succ, Chain, BlockFilter)<br>
+ && isProfitableToTailDup(BB, Succ, AdjustedSumProb, Chain,<br>
+ BlockFilter)) {<br>
----------------<br>
</span><span>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/D2858<wbr>3</a><br>
<br>
<br>
<br>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div></div></span></div>
</blockquote></div></div><br></div></div>
</blockquote></div><br></div></div></div>