[PATCH] D16440: [ThinLTO] Link in only necessary DICompileUnit operands
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 15 14:12:32 PDT 2016
On Tue, Mar 15, 2016 at 1:48 PM, Teresa Johnson via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
>
> On Thu, Mar 10, 2016 at 3:16 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> dblaikie added inline comments.
>>
>
> Thanks for the comments! I had hoped to address these all before I left
> town, but ran out of time. A few responses below, will address the rest
> when I get back!
>
Sure thing. No rush - my review approach is fairly "fire and forget until
it comes around again".
>
>
>>
>> ================
>> Comment at: lib/Linker/IRMover.cpp:1386
>> @@ +1385,3 @@
>> + // that is needed, and we should only invoke this for needed composite
>> types.
>> + assert(RMI != RetainMap.end());
>> + // If we already created a new composite type declaration, use it.
>> ----------------
>> I'm not sure this is true. Here's a type you might have to import that is
>> not in the retained types list:
>>
>> namespace {
>> struct foo { };
>> void f(foo) { }
>> }
>> struct bar { };
>> void f(bar) { }
>> void f() {
>> f(foo());
>> f(bar());
>> }
>>
>> The 'foo' type is not in the retained types list, but it may need to be
>> imported if 'f(foo)' is imported, yes?
>>
>
> While foo isn't in the retained types list, it is imported as it is
> reached via the DISubprogram for f(foo) and we do pull it in (confirmed
> with the above example and this patch). Perhaps the comment could be
> clarified to refer to needed retained types, not composite types, since
> this is only dealing with types reached via the retained types list.
>
Hmm, following the algorithm is a bit difficult for me. The code appears
more complex than I would've expected.
Perhaps I can explain the algorithm I would expect & you can point out
where I've oversimplified/got confused.
Basically what I'd expect is a loop over imported functions (OK, I see
something like that - a loop over subprograms, skipping any "unneeded"
subprograms around line 1474*).
For each imported subprogram, I'd imagine we would clone the subprogram,
and any types (as declarations) that we need.
If an imported type requires another type, I imagine the act of importing
the first type would import the second
If any imported type is referenced by a type ref/string name, I expect to
resolve that and import it
But it seems like that's not quite the way the algorithm works (notably -
importing string and direct reference types aren't handled uniformly by the
type importing step - they're deferred into a list then handled separately,
but not quite understanding why?)
* What's the longer case there that says something about "for non post-pass
metadata linking" (1445-1466)?
>
>
>> ================
>> Comment at: lib/Linker/IRMover.cpp:1398
>> @@ +1397,3 @@
>> + Composite->getFile(), Composite->getLine(), NewScope, NewBaseType,
>> + Composite->getSizeInBits(), Composite->getAlignInBits(),
>> + Composite->getOffsetInBits(), Composite->getFlags() |
>> DINode::FlagFwdDecl,
>> ----------------
>> Pretty sure you don't need the base type, size, alignment, and offset in
>> a declaration - check equivalent declarations that are generated by clang?
>> (check that the declarations this code creates look like natural
>> declarations in the original source language, etc)
>>
>
> They do seem to have size and alignment, but not baseType or offset. Will
> fix. Here's an example:
>
> !1215 = !DICompositeType(tag: DW_TAG_class_type, name: "DOMText", scope:
> !524, file: !1216, line: 90, size: 64, align: 64, flags: DIFlagFwdDecl,
> identifier: "_ZTSN11xercesc_2_57DOMTextE")
>
Oh, bother. Hrm. I wonder how those got there. I don't think they should
be. If we only have a declaration for a type there's no way we know the
correct size or alignment in general.
>
>
>> ================
>> Comment at: lib/Linker/IRMover.cpp:1411
>> @@ +1410,3 @@
>> +Metadata *
>> +IRLinker::importType(Metadata *Ty,
>> + DenseMap<DICompositeType *, DICompositeType *>
>> &RetainMap,
>> ----------------
>> This name seems a bit misleading. If I'm reading the code correctly, this
>> is used for any type's scope context - which might not be a type at all.
>> (it could be a namespace, for example) - and I assume the non-type cases
>> come out in the "map the type metadata normally" part?
>>
>
> Correct, it will also handle DINamespace which will be mapped normally via
> MapMetadata. I will rename this to something better.
>
>
>>
>> ================
>> Comment at: lib/Linker/IRMover.cpp:1427
>> @@ +1426,3 @@
>> +
>> +void IRLinker::linkImportedCompileUnit() {
>> + NamedMDNode *SrcCompileUnits = SrcM.getNamedMetadata("llvm.dbg.cu");
>> ----------------
>> Name seems a bit off - this links in potentially multiple compile units,
>> not just a single one.
>>
>
> Right, will make the name plural.
>
> This is as far as I got. Will address the rest when I am back in town
> Monday!
>
>
>>
>> ================
>> Comment at: lib/Linker/IRMover.cpp:1444
>> @@ +1443,3 @@
>> + std::vector<DICompositeType *> RetainWorklist;
>> + if (!IsMetadataLinkingPostpass) {
>> + for (DIType *Ty : CU->getRetainedTypes()) {
>> ----------------
>> What's postpass metadata linking? (what's the alternative?)
>>
>> ================
>> Comment at: lib/Linker/IRMover.cpp:1445
>> @@ +1444,3 @@
>> + if (!IsMetadataLinkingPostpass) {
>> + for (DIType *Ty : CU->getRetainedTypes()) {
>> + // For non-postpass metadata linking, any metadata referenced
>> ----------------
>> I'm not really following why we need to iterate the retained types list -
>> wouldn't we import everything starting from the subprograms we needed to
>> import, not the retained types list?
>>
>> ================
>> Comment at: lib/Linker/IRMover.cpp:1534
>> @@ +1533,3 @@
>> + stripNullSubprograms(NewCU);
>> + DestCompileUnits->addOperand(NewCU);
>> + }
>> ----------------
>> What if nothing was imported from this CU?
>>
>> It seems to me we'd want to start at the imported functions, follow their
>> DISubprograms, and import that way, rather than walking all the CUs and
>> subprograms - maybe?
>>
>>
>> http://reviews.llvm.org/D16440
>>
>>
>>
>>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson at google.com |
> 408-460-2413
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160315/5b8f86b5/attachment-0001.html>
More information about the llvm-commits
mailing list