[PATCH] D16440: [ThinLTO] Link in only necessary DICompileUnit operands

[Teresa Johnson via llvm-commits]

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!


>
> ================
> 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.


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


> ================
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160315/b431183a/attachment.html>