<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 25, 2017 at 4:32 PM, 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 class=""><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" target="_blank">davidxl@google.com</a>> wrote:<br type="attribution"><blockquote class="m_4639382685922484009quote" 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_4639382685922484009quoted-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_4639382685922484009m_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_4639382685922484009m_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></span><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. </div></div></blockquote><div><br></div><div>We always run static branch prediction without PGO, so if there is any information in source order that can help guide block layout, this logic should be handled in the static prediction heuristics. Embedding knowledge here is not the right thing to do IMO.</div><div><br></div><div>Besides, it is the CFG successor order you are preserving (relative to the selected successor without taildup), not the source order. This seems rather arbitrary -- it is not impossible for FE to swap the branch condition and change the order. In your code, it seems to say if the tailDup candidate has the same probability as the selected successor but its position is after it, it will be skipped -- that does not sound right either -- what if the isProfitableToDup returns true?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div dir="auto">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></blockquote><div><br></div><div><br></div><div>Have you actually seen a case where there is a tie you need to rely on this heuristic? Also would it better to let 'isProfitableToDup to return the actual profit of taildup such that the one with the largest benefit will be selected?</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"><div dir="auto"><span class=""><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="m_4639382685922484009quote" 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_4639382685922484009quoted-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_4639382685922484009m_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_4639382685922484009m_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></span></div>
</blockquote></div><br></div></div>