<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:Helvetica;
panose-1:2 11 6 4 2 2 2 2 2 4;}
@font-face
{font-family:Helvetica;
panose-1:2 11 6 4 2 2 2 2 2 4;}
@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.MsoAcetate, li.MsoAcetate, div.MsoAcetate
{mso-style-priority:99;
mso-style-link:"Balloon Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:8.0pt;
font-family:"Tahoma","sans-serif";}
span.gmailmsg
{mso-style-name:gmail_msg;}
span.EmailStyle18
{mso-style-type:personal;
font-family:"Calibri","sans-serif";
color:windowtext;}
span.BalloonTextChar
{mso-style-name:"Balloon Text Char";
mso-style-priority:99;
mso-style-link:"Balloon Text";
font-family:"Tahoma","sans-serif";}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@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 bgcolor="white" 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">For DWARF, all "line 0" records are semantically equivalent. However there is a size cost in the encoded line table if an explicit "line 0" has a different
file (or column) than the previous or following line record.<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"><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""> Taewook Oh [mailto:twoh@fb.com]
<br>
<b>Sent:</b> Thursday, February 23, 2017 12:02 PM<br>
<b>To:</b> Adrian Prantl; David Blaikie<br>
<b>Cc:</b> reviews+D29833+public+d0b46dd83e56e03f@reviews.llvm.org; danielcdh@gmail.com; echristo@gmail.com; rnk@google.com; rob.lougher@gmail.com; Robinson, Paul; andrea.dibiagio@gmail.com; llvm-commits@lists.llvm.org; mehdi.amini@apple.com<br>
<b>Subject:</b> Re: [PATCH] D29833: Improve the API of DILocation::getMergedLocation()<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">I think the existing implementation of getMergedLocation implies that if two debug locations can be discriminated, we should return line:0 location than keep the original
location. The gist of this patch is returning line:0 locations instead of nullptr for call instructions, I believe.
<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Regarding API, I think “I1->setMergedDebugLoc(I1->getDebugLoc(), I2->getDebugLoc()” is better than “I1->setDebugLoc(DILocation::getMergedLocation(I1->getDebugLoc(), I2->getDebugLoc()))”,
because it is more succinct, and we can handle CallInst separately. <o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Another issue we’ve been discussed was what should be the “scope” of returning line:0 location, but I actually don’t have strong opinion about it, because with line:0 it
is already clear that the DebugLoc is dummy (Maybe I missed some point here). <o:p>
</o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Taewook<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><o:p> </o:p></span></p>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-family:"Calibri","sans-serif";color:black">From:
</span></b><span style="font-family:"Calibri","sans-serif";color:black"><<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> on behalf of Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>><br>
<b>Date: </b>Thursday, February 23, 2017 at 10:52 AM<br>
<b>To: </b>David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
<b>Cc: </b>"<a href="mailto:reviews+D29833+public+d0b46dd83e56e03f@reviews.llvm.org">reviews+D29833+public+d0b46dd83e56e03f@reviews.llvm.org</a>" <<a href="mailto:reviews+D29833+public+d0b46dd83e56e03f@reviews.llvm.org">reviews+D29833+public+d0b46dd83e56e03f@reviews.llvm.org</a>>,
"<a href="mailto:danielcdh@gmail.com">danielcdh@gmail.com</a>" <<a href="mailto:danielcdh@gmail.com">danielcdh@gmail.com</a>>, "<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>" <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>>, "<a href="mailto:rnk@google.com">rnk@google.com</a>"
<<a href="mailto:rnk@google.com">rnk@google.com</a>>, "<a href="mailto:rob.lougher@gmail.com">rob.lougher@gmail.com</a>" <<a href="mailto:rob.lougher@gmail.com">rob.lougher@gmail.com</a>>, "<a href="mailto:paul.robinson@sony.com">paul.robinson@sony.com</a>"
<<a href="mailto:paul.robinson@sony.com">paul.robinson@sony.com</a>>, Taewook Oh <<a href="mailto:twoh@fb.com">twoh@fb.com</a>>, "<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>" <<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>>,
"<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>" <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>>, Mehdi AMINI <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>><br>
<b>Subject: </b>Re: [PATCH] D29833: Improve the API of DILocation::getMergedLocation()<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<p class="MsoNormal">On Feb 23, 2017, at 10:08 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<o:p></o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">On Thu, Feb 23, 2017 at 9:35 AM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<o:p></o:p></span></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">
<div>
<div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">On Feb 23, 2017, at 9:13 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<o:p></o:p></span></p>
</div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">You were suggesting that the nice thing about the former was that the user couldn't fail to have a debug location:<br>
<br>
<span class="gmailmsg"><span style="color:#212121">"The primary benefit of having the API on Instruction is that we can make it impossible to accidentally create a locationless call instruction in a function with debuginfo (which would crash/assert the backend
when creating inline scopes)."</span></span><span style="color:#212121"><br>
</span><br>
Which I'm a bit fuzzy on - if a new call instruction is created it'd still have no location so I'm not sure this choice of API provides extra defense against that mistake.<br>
<br>
If, today, an existing call instruction is moved then the location zero'd out - it's going to need work to replace the zeroing with merging no matter whether the API for that merging is on DebugLoc or Instruction.<o:p></o:p></span></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
</div>
</div>
</div>
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">Agreed.<o:p></o:p></span></p>
</div>
</div>
</div>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><br>
So I guess coming back to your point: could you explain in more detail how having the API on Instruction provides a stronger guarantee/safety than having it elsewhere? I'm not clear on what that's defending against.<o:p></o:p></span></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
</div>
</div>
</div>
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">I want to make it harder to accidentally assign a null DebugLoc to a call. Currently getMergedDebugLoc() returns a null location (to save space in the line table) and at
each call site we have to write extra code to create a line:0 location if the instruction we are creating the merged location for is a call. See <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D29833&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=kOsLCgQzH7N8ptZ7diJD9g&m=O6D-OeqGm2sNUSI30Qqz7sFjmurpqgcX9CVy6OgkNGw&s=_XNi_qHqSnfqrjV9rLI5uU6TiIsFhrAykuq40uy9pJQ&e=" target="_blank">https://reviews.llvm.org/D29833</a>, lib/Transforms/Utils/SimplifyCFG.cpp:1280
for an example — though upon closer inspection the original code was just keeping the original location of the call instead of assigning a line:0 location.<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">I though it would be nice to hide this detail in the API, but this really makes it look like we want to decide how to handle call instructions (assign line:0, keep original
loc) on a case-by-case basis.<o:p></o:p></span></p>
</div>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">Oh, right, sorry - sure, seems reasonable to make it part of Instruction so it can Do The Right Thing depending on whether the Instruction is a Call. (maybe a "setMergedLocation(DebugLoc,
DebugLoc)" that ignores the current location - I suppose in theory it could be a variadic function to handle more than two locations, but that probably doesn't come up often enough to worry? Not sure))<o:p></o:p></span></p>
</div>
</div>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Yes, but then I realized that it is not obvious what The Right Thing is. When the instruction is a call, should we return a line:0 location, should we keep the original location? Is there one answer that fits all use-cases? I’m afraid now
that we would be hiding the fact that the user needs to do make an informed decision here behind the new API.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">You mentioned abandoning this in favor of the other change under review? Seems the discussion is here so we might as well continue it here.<o:p></o:p></span></p>
</div>
</div>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<div>
<p class="MsoNormal">Sure, works for me.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">-- adrian<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">- Dave<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""> <o:p></o:p></span></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">
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">thanks,<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">adrian<o:p></o:p></span></p>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
</div>
</div>
</body>
</html>