<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Dec 5, 2018 at 1:52 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Dec 5, 2018 at 11:37 AM Teresa Johnson via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">tejohnson accepted this revision.<br>
tejohnson added a comment.<br>
This revision is now accepted and ready to land.<br>
<br>
LGTM to address regression, especially since the debug info already isn't maintained for global variables that are const propagated. David, as a follow on it would be good to understand if/when there are cases where the debug info should be imported for these variables (i.e. if they aren't in fact constant propagated after internalization).<br></blockquote><div><br>Sure - would you prefer that in the form of an email discussion, documentation, further elaborated comments (might get a bit verbose, but I did try to cover some of the tricks there)?<br></div></div></div></blockquote><div><br></div><div>That's a good question. I suppose an email discussion around cases where we will now be losing information after importing and what we could do to fix/improve that would be good. FIXME comments might be good too?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>(as an aside: We did some tricks to ensure that ThinLTO+Split DWARF only produces a single CU (for a few reasons - Split DWARF wasn't really well defined with multiple CUs, GDB couldn't cope with it, and especially it didn't have any well defined way of doing cross-CU references to describe the importing anyway), but looking at how much this regression grew debug info - I wonder if anyone (even those not using Split DWARF) would really want all the little CU fragments to be emitted as separate CUs in each ThinLTO object file - it looks not great, even if it is somewhat more accurate to the source code (as best it can in the medium term at least) - DWARF isn't really built for all these tiny CUs - though if Apple folks haven't been complaining about the debug info size for their ThinLTO use... maybe that's good enough?)<br></div></div></div></blockquote><div><br></div><div>That's a good question. I haven't heard any complaints, but not everyone will notice. My understanding of Apples debug info situation is that they essentially split it out and link it separately, so it might not be as noticeably problematic there?</div><div><br></div><div>On the flip side, I wonder if it would be noticeably bad to always avoid all that extra debug info if it isn't significantly more useful to have them.</div><div> </div><div>Teresa</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<br>
================<br>
Comment at: lib/Linker/IRMover.cpp:1065<br>
     ValueMap.MD()[CU->getRawRetainedTypes()].reset(nullptr);<br>
+    ValueMap.MD()[CU->getRawGlobalVariables()].reset(nullptr);<br>
<br>
----------------<br>
Please add a comment here (the old one that was removed isn't quite accurate now, but could probably be modified).<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D55309/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D55309/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D55309" rel="noreferrer" target="_blank">https://reviews.llvm.org/D55309</a><br>
<br>
<br>
<br>
</blockquote></div></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"><br></td></tr></tbody></table></span></div></div></div></div>