<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">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><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><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>