<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;}
span.EmailStyle17
{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">I talked to Matthew this morning about this debate. He will update the patch to use the merge API and repost it.<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""> David Blaikie [mailto:dblaikie@gmail.com]
<br>
<b>Sent:</b> Tuesday, October 24, 2017 10:01 AM<br>
<b>To:</b> Adrian Prantl<br>
<b>Cc:</b> Robinson, Paul; reviews+D38088+public+4bc8f3de9212eeb7@reviews.llvm.org; Voss, Matthew; craig.topper@gmail.com; echristo@gmail.com; llvm-commits@lists.llvm.org<br>
<b>Subject:</b> Re: [PATCH] D38088: Fix out-of-order stepping behavior in programs with hoisted constants.<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Tue, Oct 24, 2017 at 9:52 AM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.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">
<div>
<div>
<p class="MsoNormal"><br>
<br>
<o:p></o:p></p>
<div>
<p class="MsoNormal">On Oct 24, 2017, at 9:23 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">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 Tue, Oct 24, 2017 at 9:20 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">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>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><br>
<br>
<o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">On Oct 24, 2017, at 9:12 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" 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 Tue, Oct 24, 2017 at 9:08 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">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>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><br>
<br>
<o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">On Oct 24, 2017, at 9:05 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"">It merges as well as hoisting<o:p></o:p></span></p>
</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>
</div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">Ok. But re-reading the motivation of the patch, it looks like merging the locations (let's assume they are identical) would not actually solve the issue the hoisted constant's
location being unexpected, right?<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">Were you suggesting to merge the location of the constant with the location of the InsertPoint?<o:p></o:p></span></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><br>
Yeah, I'm really not sure how to address it - if the constant is used in two different basic blocks but has the same location in both, but the constant is hoisted into a third basic block? We probably want to null out the location in that case (so I'm not sure
merging it with the insertion point is ideal - letting the insertion point's location flow into this instruction if that's how it shakes out in the backend is fine, but actually specifying that location seems a bit off - if the instruction gets moved around
some more, etc)<br>
<o:p></o:p></span></p>
</div>
</div>
</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>
</div>
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">Yes. Based on what the patch is trying to do the current implementation seems right.<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""><br>
I guess the outstanding question I have is: is it worth trying to merge the location in the cases where it isn't hoisted into some other basic block? Any thoughts/strong feelings? Otherwise I'll probably just accept it as-is, even if the constant's used in
two places in a single basic block and could have its location preserved witohut tainting profiling data.<o:p></o:p></span></p>
</div>
</div>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
<div>
<div>
<div>
<p class="MsoNormal">Ah I see. The distinction between it being hoisted into some other basic block is relevant for PGO because it only confuses PGO when the constant is moved to a different basic block. Yes, it would be better to merge to locations in that
case, since it doesn't affect the PGO workflow or stepping in the debugger negatively, so we shouldn't throw away the location.<o:p></o:p></p>
</div>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><br>
Well it can/does still make for weird stepping behavior, I think - I think the constant can get hoisted to the start of the basic block, not just the first use (can anyone confirm this? If it only gets hoisted to the first use then, within a basic block, merging
the locations would be fine - since you were already going to step there before this transformation) - so it could move far from any of the constant's uses.<br>
<br>
- Dave<br>
<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">
<div>
<div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">-- adrian<o:p></o:p></p>
</div>
</div>
</div>
<div>
<div>
<p class="MsoNormal"><br>
<br>
<o:p></o:p></p>
<div>
<div>
<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-right:0in">
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">In the long term I don't think we should be erasing accurate (but confusing) source locations, and instead (for example) be more principled about what we mark with is_stmt
in the linetable, so we could have multiple classes of line table entries and a debugger may choose to ignore the more accurate ones if it messes with stepping (but have them available for crashes/breakpoints/etc). I'm not sure if this also applies exactly
the same way to profilers, but in the worst case we could add another flag to the line table to mark is_interesting_profiler_location.<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""><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>
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
</div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><br>
<br>
<o:p></o:p></span></p>
<div>
<div>
<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>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><o:p> </o:p></span></p>
</div>
</div>
<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>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><br>
<br>
<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"">On Tue, Oct 24, 2017 at 9:00 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">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">
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif""><br>
<br>
> On Oct 24, 2017, at 8:48 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Sep 26, 2017 at 2:05 PM Robinson, Paul <<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>> wrote:<br>
> Doesn't need to be merged with the insertion point, I don't think? Does this merge across basic blocks? or just raise the uses of a constant to the beginning of a single basic block?<br>
><br>
><br>
><br>
> I read the pass as looking for a dominating insertion point across multiple basic blocks. Doesn't mean the insertion point necessarily is in a different block, but I *think* it could be.<br>
><br>
><br>
> Yeah, still seems like it might be nice to keep the location if it doesn't cross a basic block boundary.<br>
><br>
> Adrian - any ideas/thoughts on all this?<br>
<br>
Looks like I missed the original review... Does Constant Hoisting just move a single constant to the top, or does it merge multiple identical constants (while hoisting them)? In the latter case getMergedLocation should definitely be used. In the former case
I'm not so sure if erasing the location is the right thing to do in the first place (but it probably doesn't hurt much in practice). So I guess my question is: What are you suggesting should the location be merged with?<br>
<br>
-- adrian<o:p></o:p></span></p>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</body>
</html>