<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Feb 23, 2017 at 10:52 AM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Feb 23, 2017, at 10:08 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-7246607823876746492Apple-interchange-newline gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><br class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Thu, Feb 23, 2017 at 9:35 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" class="gmail_msg" target="_blank">aprantl@apple.com</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Feb 23, 2017, at 9:13 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="gmail_msg m_-7246607823876746492m_600931650097736089Apple-interchange-newline"><div class="gmail_msg"><div dir="ltr" class="gmail_msg">You were suggesting that the nice thing about the former was that the user couldn't fail to have a debug location:<br class="gmail_msg"><br class="gmail_msg"><span class="gmail_msg" style="color:rgb(33,33,33)">"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)."<br class="gmail_msg"></span><br class="gmail_msg">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 class="gmail_msg"><br class="gmail_msg">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.<br class="gmail_msg"></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><div class="gmail_msg">Agreed.</div></div></div><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" class="gmail_msg"><br class="gmail_msg">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.</div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><div class="gmail_msg">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://reviews.llvm.org/D29833" class="gmail_msg" 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.</div><div class="gmail_msg">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.</div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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))<br class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg">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.</div></div></div></blockquote><div><br></div><div>When merging there's no option to keep the original (due to profile requirements, at the very least (and arguably debug quality - but that's harder to be sure of)). I believe the zero location (does DWARF say that the file is still relevant when the line is zero? I hope not). <br><br>Scope is still unclear, to be sure. Really a null scope would be great (so it'd just be whatever scope was nearby - like having no location works today), but that might take some working.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">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.<br class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"></div><div class="gmail_msg">Sure, works for me.</div><div class="gmail_msg">-- adrian</div><div class="gmail_msg"><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">- Dave</div><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div class="gmail_msg" style="word-wrap:break-word"><div class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">thanks,</div><div class="gmail_msg">adrian</div></div></div></blockquote></div></div></div></blockquote></div><br class="gmail_msg"></div></blockquote></div></div>