<html 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="Title" content="">
<meta name="Keywords" content="">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:"맑은 고딕";}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman";}
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;}
span.gmailmsg
{mso-style-name:gmail_msg;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:Calibri;
color:windowtext;}
span.msoIns
{mso-style-type:export-only;
mso-style-name:"";
text-decoration:underline;
color:teal;}
.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>
</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">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"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:Calibri">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"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:Calibri">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"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:Calibri">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:Calibri">Taewook<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:Calibri"><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;color:black">From: </span>
</b><span style="font-family:Calibri;color:black"><aprantl@apple.com> on behalf of Adrian Prantl <aprantl@apple.com><br>
<b>Date: </b>Thursday, February 23, 2017 at 10:52 AM<br>
<b>To: </b>David Blaikie <dblaikie@gmail.com><br>
<b>Cc: </b>"reviews+D29833+public+d0b46dd83e56e03f@reviews.llvm.org" <reviews+D29833+public+d0b46dd83e56e03f@reviews.llvm.org>, "danielcdh@gmail.com" <danielcdh@gmail.com>, "echristo@gmail.com" <echristo@gmail.com>, "rnk@google.com" <rnk@google.com>, "rob.lougher@gmail.com"
<rob.lougher@gmail.com>, "paul.robinson@sony.com" <paul.robinson@sony.com>, Taewook Oh <twoh@fb.com>, "andrea.dibiagio@gmail.com" <andrea.dibiagio@gmail.com>, "llvm-commits@lists.llvm.org" <llvm-commits@lists.llvm.org>, Mehdi AMINI <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>
</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"><o:p> </o:p></span></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica">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-right:0in">
<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">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"><o:p> </o:p></span></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica">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"><o:p> </o:p></span></p>
</div>
</div>
</div>
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica">Agreed.<o:p></o:p></span></p>
</div>
</div>
</div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica"><br>
<br>
<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"><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"><o:p> </o:p></span></p>
</div>
</div>
</div>
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica">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">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"><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica">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">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"><br>
<br>
<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">- Dave<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica"> <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-right:0in">
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica"><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica">thanks,<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Helvetica">adrian<o:p></o:p></span></p>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
<p class="MsoNormal"><br>
<br>
<o:p></o:p></p>
</div>
</body>
</html>