[PATCH] D55309: ThinLTO: Do not import debug info for imported global constants

[Teresa Johnson via llvm-commits]

On Wed, Dec 5, 2018 at 1:52 PM David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Wed, Dec 5, 2018 at 11:37 AM Teresa Johnson via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> tejohnson accepted this revision.
>> tejohnson added a comment.
>> This revision is now accepted and ready to land.
>>
>> 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).
>>
>
> 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)?
>

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?


> (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?)
>

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?

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.

Teresa


>
>>
>>
>>
>> ================
>> Comment at: lib/Linker/IRMover.cpp:1065
>>      ValueMap.MD()[CU->getRawRetainedTypes()].reset(nullptr);
>> +    ValueMap.MD()[CU->getRawGlobalVariables()].reset(nullptr);
>>
>> ----------------
>> Please add a comment here (the old one that was removed isn't quite
>> accurate now, but could probably be modified).
>>
>>
>> Repository:
>>   rL LLVM
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D55309/new/
>>
>> https://reviews.llvm.org/D55309
>>
>>
>>
>>

-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181206/1ec89da1/attachment.html>