<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 19, 2016 at 11:30 AM Robert Lougher <<a href="mailto:rob.lougher@gmail.com">rob.lougher@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_msg">Yes, we could use a new discriminator to differentiate the common-tail from the original blocks. As there will be no source correspondence the counts will be ignored by the sample loader (it maps from source). Talking to the debugger people it seems they don't take any notice of the discriminator so the fact it doesn't match the source won't be a problem. However, I don't know if this is true for all consumers. The question is, is it worth doing for what is a fairly rare special case?<br class="gmail_msg"></div></div></blockquote><div><br></div><div>For my money I doubt it's worthwhile</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"></div><br class="gmail_msg">Thanks,<br class="gmail_msg">Rob.<br class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div></div><div class="gmail_extra gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg">On 19 October 2016 at 16:51, Robinson, Paul <span dir="ltr" class="gmail_msg"><<a href="mailto:paul.robinson@sony.com" class="gmail_msg" target="_blank">paul.robinson@sony.com</a>></span> wrote:<br class="gmail_msg"><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div link="blue" vlink="purple" lang="EN-US" class="gmail_msg">
<div class="m_-6763102085933274283m_5106709760416552173WordSection1 gmail_msg"><span class="gmail_msg">
<p class="MsoNormal gmail_msg" style="margin-left:.5in">
<span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">Discriminators can't help you when the same instruction belongs to two different blocks.</span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="MsoNormal gmail_msg" style="margin-left:.5in"><br class="gmail_msg">
What I meant was - you were talking about the ambiguity of keeping it on the same line, when sunk into the common tail block (if it happens to be on the same line) - that that would make the profile ambiguous/confused because now all 3 blocks are the same location.
Discriminators disambiguate those 3 blocks that are all on the same line (the if, the else, and the common successor).<br class="gmail_msg">
<br class="gmail_msg">
<span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg"><u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
</span><p class="MsoNormal gmail_msg"><a name="m_-6763102085933274283_m_5106709760416552173__MailEndCompose" class="gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">Discriminators let you say that two different instructions belong to two different blocks even though they have the same source location.
If we ignore columns for a single-line if-then-else then yes we can (do!) use discriminators exactly that way.<u class="gmail_msg"></u><u class="gmail_msg"></u></span></a></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg"><u class="gmail_msg"></u> <u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">If we postulate a transformation from
<u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg" style="text-indent:.5in"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">if (x) stmt1; else stmt2;<u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">into<u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg" style="text-indent:.5in"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">{ if (x) stmt1-prefix; else stmt2-prefix; common-suffix; }<u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">then you could in principle use discriminators to distinguish the original two blocks and the artificial third block.<u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">However that artificial third block does not unambiguously map back to any source block. It does not help weight the branch decision that chooses between stmt1
and stmt2, and the *relative* weight is really what matters here.<u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg"><u class="gmail_msg"></u> <u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">I don't know what the profile analysis does with hits on the common suffix; one hopes that they are somehow treated the same as hits on the instructions that
are evaluating the 'x' condition, i.e. counting toward the block containing the original 'if'. With sufficient bookkeeping I suppose we'd be able to tag the common suffix with the source location of the parent 'if' but it's doubtful we could really make that
work.<u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">--paulr<u class="gmail_msg"></u><u class="gmail_msg"></u></span></p>
<p class="MsoNormal gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg"><u class="gmail_msg"></u> <u class="gmail_msg"></u></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt" class="gmail_msg">
<div class="gmail_msg">
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in" class="gmail_msg">
<p class="MsoNormal gmail_msg"><b class="gmail_msg"><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"" class="gmail_msg">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"" class="gmail_msg"> David Blaikie [mailto:<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>]
<br class="gmail_msg">
<b class="gmail_msg">Sent:</b> Tuesday, October 18, 2016 5:51 PM<br class="gmail_msg">
<b class="gmail_msg">To:</b> Robinson, Paul; <a href="mailto:reviews%2BD25742%2Bpublic%2B84a9fdd1c5228b3b@reviews.llvm.org" class="gmail_msg" target="_blank">reviews+D25742+public+84a9fdd1c5228b3b@reviews.llvm.org</a>; <a href="mailto:rob.lougher@gmail.com" class="gmail_msg" target="_blank">rob.lougher@gmail.com</a>; <a href="mailto:danielcdh@gmail.com" class="gmail_msg" target="_blank">danielcdh@gmail.com</a>; Pieb, Wolfgang; <a href="mailto:aprantl@apple.com" class="gmail_msg" target="_blank">aprantl@apple.com</a></span></p><div class="gmail_msg"><div class="m_-6763102085933274283h5 gmail_msg"><br class="gmail_msg">
<b class="gmail_msg">Cc:</b> <a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a><br class="gmail_msg">
<b class="gmail_msg">Subject:</b> Re: [PATCH] D25742: Remove debug location from common tail when tail-merging<u class="gmail_msg"></u><u class="gmail_msg"></u></div></div><p class="gmail_msg"></p>
</div>
</div><div class="gmail_msg"><div class="m_-6763102085933274283h5 gmail_msg">
<p class="MsoNormal gmail_msg"><u class="gmail_msg"></u> <u class="gmail_msg"></u></p>
<div class="gmail_msg">
<p class="MsoNormal gmail_msg" style="margin-bottom:12.0pt"><u class="gmail_msg"></u> <u class="gmail_msg"></u></p>
<div class="gmail_msg">
<div class="gmail_msg">
<p class="MsoNormal gmail_msg">On Tue, Oct 18, 2016 at 5:04 PM Robinson, Paul <<a href="mailto:paul.robinson@sony.com" class="gmail_msg" target="_blank">paul.robinson@sony.com</a>> wrote:<u class="gmail_msg"></u><u class="gmail_msg"></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in" class="gmail_msg">
<div class="gmail_msg">
<div class="gmail_msg">
<p class="MsoNormal gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">Discriminators can't help you when the same instruction belongs to two different
blocks.</span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
</div>
</div>
</blockquote>
<div class="gmail_msg">
<p class="MsoNormal gmail_msg"><br class="gmail_msg">
What I meant was - you were talking about the ambiguity of keeping it on the same line, when sunk into the common tail block (if it happens to be on the same line) - that that would make the profile ambiguous/confused because now all 3 blocks are the same location.
Discriminators disambiguate those 3 blocks that are all on the same line (the if, the else, and the common successor).<br class="gmail_msg">
<br class="gmail_msg">
But anyway - there's no great answer for any of this, but I'm happy enough to use profiling as a guide for what the right location is (even if, arguably, the experience for debugger users might be a bit weird - not having certain lines of code execute, etc
- because the alternative would just be differently weird). & I don't think it's really worth optimizing for the "these two blocks happen to be on the same line so we can use the line and drop the column"<br class="gmail_msg">
<br class="gmail_msg">
- Dave<br class="gmail_msg">
<u class="gmail_msg"></u><u class="gmail_msg"></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in" class="gmail_msg">
<div class="gmail_msg">
<div class="gmail_msg">
<p class="MsoNormal gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg"> </span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="MsoNormal gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">The point (which we should not lose track of) is that when you merge the tails
of two blocks, the source attribution becomes inherently ambiguous. This is not great for debugging and is bad for profiling. Erasing the source attribution means at least we aren't actively lying to either one; again it is not great for debugging, although
IIUC profiling can cope. In my opinion that makes erasing the source location "less bad" than what we have now.</span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="MsoNormal gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg"> </span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="MsoNormal gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">But, the alternatives appear to be either killing off the optimization (removing
the ambiguity at a code-size cost) or doing a LOT of work so we can attribute multiple source locations to the same instructions (which only pushes the problem downstream to the DWARF consumer, who probably has no idea how to cope). (It is not, I have discovered,
impossible to construct a line table that attributes multiple source locations to the same instruction. But nobody expects to see that.)</span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
</div>
</div>
</blockquote>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in" class="gmail_msg">
<div class="gmail_msg">
<div class="gmail_msg">
<p class="MsoNormal gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg">--paulr</span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="MsoNormal gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg"> </span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="MsoNormal gmail_msg"><a name="m_-6763102085933274283_m_5106709760416552173_m_7169161408211231810__MailEndCompose" class="gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d" class="gmail_msg"> </span></span></a><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt" class="gmail_msg">
<div class="gmail_msg">
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in" class="gmail_msg">
<p class="MsoNormal gmail_msg"><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><b class="gmail_msg"><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"" class="gmail_msg">From:</span></b></span><span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"" class="gmail_msg">
llvm-commits [mailto:<a href="mailto:llvm-commits-bounces@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits-bounces@lists.llvm.org</a>]
<b class="gmail_msg">On Behalf Of </b>David Blaikie via llvm-commits</span></span><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"" class="gmail_msg"><br class="gmail_msg">
<span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><b class="gmail_msg">Sent:</b> Tuesday, October 18, 2016 4:34 PM</span><br class="gmail_msg">
<span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><b class="gmail_msg">To:</b> <a href="mailto:reviews%2BD25742%2Bpublic%2B84a9fdd1c5228b3b@reviews.llvm.org" class="gmail_msg" target="_blank">
reviews+D25742+public+84a9fdd1c5228b3b@reviews.llvm.org</a>; <a href="mailto:rob.lougher@gmail.com" class="gmail_msg" target="_blank">
rob.lougher@gmail.com</a>; <a href="mailto:danielcdh@gmail.com" class="gmail_msg" target="_blank">danielcdh@gmail.com</a>; Pieb, Wolfgang;
<a href="mailto:aprantl@apple.com" class="gmail_msg" target="_blank">aprantl@apple.com</a></span><br class="gmail_msg">
<span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><b class="gmail_msg">Cc:</b> <a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">
llvm-commits@lists.llvm.org</a></span><br class="gmail_msg">
<span class="m_-6763102085933274283m_5106709760416552173gmailmsg gmail_msg"><b class="gmail_msg">Subject:</b> Re: [PATCH] D25742: Remove debug location from common tail when tail-merging</span></span><u class="gmail_msg"></u><u class="gmail_msg"></u></p>
</div>
</div>
</div>
</div>
</div>
<div class="gmail_msg">
<div class="gmail_msg">
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt" class="gmail_msg">
<p class="MsoNormal gmail_msg"> <u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="m_-6763102085933274283m_5106709760416552173gmailmsg1 gmail_msg">That's what discriminators are for - but I've no idea where that fits in the workflow etc.<u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="m_-6763102085933274283m_5106709760416552173gmailmsg1 gmail_msg">In any case I'm not sure it's worth worrying about reusing the same line when it happens to be all on the same line (& removing the column - what a debugger would do seeing some things with column info and some without etc)<u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<p class="MsoNormal gmail_msg"> <u class="gmail_msg"></u><u class="gmail_msg"></u></p>
<div class="gmail_msg">
<div class="gmail_msg">
<p class="MsoNormal gmail_msg">On Tue., 18 Oct. 2016, 4:27 pm Paul Robinson, <<a href="mailto:paul.robinson@sony.com" class="gmail_msg" target="_blank">paul.robinson@sony.com</a>> wrote:<u class="gmail_msg"></u><u class="gmail_msg"></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt" class="gmail_msg">
<p class="MsoNormal gmail_msg" style="margin-bottom:12.0pt">probinson added a comment.<br class="gmail_msg">
<br class="gmail_msg">
In <a href="https://reviews.llvm.org/D25742#573687" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25742#573687</a>, @aprantl wrote:<br class="gmail_msg">
<br class="gmail_msg">
> In <a href="https://reviews.llvm.org/D25742#573401" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25742#573401</a>, @probinson wrote:<br class="gmail_msg">
><br class="gmail_msg">
> > In <a href="https://reviews.llvm.org/D25742#573352" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25742#573352</a>, @aprantl wrote:<br class="gmail_msg">
> ><br class="gmail_msg">
> > > This approach seems generally fine, but I have one question:<br class="gmail_msg">
> > ><br class="gmail_msg">
> > > If the code were on a single line, and both locations share a common ancestor scope, it seems make sense to create a new location using the common ancestor scope and line and only remove the column information.<br class="gmail_msg">
> ><br class="gmail_msg">
> ><br class="gmail_msg">
> > That would collapse the if-then-else into (effectively) a single statement. That probably works okay for a debugger but not profiling, which still wants to treat the then/else as distinct. And, after the tail merging, the tails are no longer distinct.<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> I'm not sure I understand your point. How is having an orphaned add instruction preferable over having it associated with the collapsed if-then-else statement? Wouldn't I want that instruction to be counted towards the line?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
No, the profiler wants to assign sample counts to each block individually. By giving each block the same source attribution, you assert that they have the same profiles. That's unlikely to be true in practice. Really what happens is that the sample counts
would be assigned to the parent block, which doesn't help the profiler sort out what to do with the nested blocks.<br class="gmail_msg">
<br class="gmail_msg">
Admittedly, zapping the source attribution on the merged (parts of the) blocks doesn't let you attribute those counts to *any* block, but at least you aren't attributing counts incorrectly.<br class="gmail_msg">
<br class="gmail_msg">
Robert may want to correct some of my assumptions here, but this is my understanding.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D25742" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25742</a><br class="gmail_msg">
<br class="gmail_msg">
<u class="gmail_msg"></u><u class="gmail_msg"></u></p>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div></div></div>
</div>
</div>
</blockquote></div><br class="gmail_msg"></div>
</blockquote></div></div>