<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
        {font-family:Calibri;
        panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
        {font-family:Tahoma;
        panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
        {margin:0in;
        margin-bottom:.0001pt;
        font-size:12.0pt;
        font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
        {mso-style-priority:99;
        color:blue;
        text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
        {mso-style-priority:99;
        color:purple;
        text-decoration:underline;}
p
        {mso-style-priority:99;
        mso-margin-top-alt:auto;
        margin-right:0in;
        mso-margin-bottom-alt:auto;
        margin-left:0in;
        font-size:12.0pt;
        font-family:"Times New Roman","serif";}
span.EmailStyle18
        {mso-style-type:personal-reply;
        font-family:"Calibri","sans-serif";
        color:#1F497D;}
.MsoChpDefault
        {mso-style-type:export-only;
        font-family:"Calibri","sans-serif";}
@page WordSection1
        {size:8.5in 11.0in;
        margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
        {page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Discriminators can't help you when the same instruction belongs to two different blocks.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">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.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">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.)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">--paulr<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></a></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> llvm-commits [mailto:llvm-commits-bounces@lists.llvm.org]
<b>On Behalf Of </b>David Blaikie via llvm-commits<br>
<b>Sent:</b> Tuesday, October 18, 2016 4:34 PM<br>
<b>To:</b> reviews+D25742+public+84a9fdd1c5228b3b@reviews.llvm.org; rob.lougher@gmail.com; danielcdh@gmail.com; Pieb, Wolfgang; aprantl@apple.com<br>
<b>Cc:</b> llvm-commits@lists.llvm.org<br>
<b>Subject:</b> Re: [PATCH] D25742: Remove debug location from common tail when tail-merging<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p>That's what discriminators are for - but I've no idea where that fits in the workflow etc.<o:p></o:p></p>
<p>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)<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Tue., 18 Oct. 2016, 4:27 pm Paul Robinson, <<a href="mailto:paul.robinson@sony.com">paul.robinson@sony.com</a>> wrote:<o:p></o:p></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">
<p class="MsoNormal" style="margin-bottom:12.0pt">probinson added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D25742#573687" target="_blank">https://reviews.llvm.org/D25742#573687</a>, @aprantl wrote:<br>
<br>
> In <a href="https://reviews.llvm.org/D25742#573401" target="_blank">https://reviews.llvm.org/D25742#573401</a>, @probinson wrote:<br>
><br>
> > In <a href="https://reviews.llvm.org/D25742#573352" target="_blank">https://reviews.llvm.org/D25742#573352</a>, @aprantl wrote:<br>
> ><br>
> > > This approach seems generally fine, but I have one question:<br>
> > ><br>
> > > 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>
> ><br>
> ><br>
> > 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>
><br>
><br>
> 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>
<br>
<br>
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>
<br>
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>
<br>
Robert may want to correct some of my assumptions here, but this is my understanding.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D25742" target="_blank">https://reviews.llvm.org/D25742</a><br>
<br>
<br>
<o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</body>
</html>