[PATCH] D14387: Move comdat setup after global value body linking in ModuleLinker

[Teresa Johnson via llvm-commits]

On Thu, Nov 5, 2015 at 8:53 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Nov-05, at 17:08, Teresa Johnson <tejohnson at google.com> wrote:
>>
>> tejohnson added a comment.
>>
>> Following up after discussing with Rafael on IRC.
>>
>> Splitting the linker will require quite a bit of work since LTO discovers what symbols it needs as it copies in function bodies.
>>
>> As an alternative to this patch:
>>
>> 1. For ThinLTO we can strip decls from comdats if we aren't importing that comdat group (note that the importer eventually needs to import the entire comdat group to get correct linker behavior).
>> 2. For LTO (and llvm-lto -only-needed) this situation only arises with my currently reverted patch http://reviews.llvm.org/D14195 to move metadata linking after all global value body linking when there is metadata that references a non-materialized global. At HEAD, a metadata reference causes the def to be linked in, which is unnecessary.
>
> Can you change metadata linking *not* to cause the def to be linked in?

Moving metadata linking will cause the def to not be linked, but I
assume you mean the decl. Yeah, let me try just nulling them out if we
find one that isn't materialized at that point. I assume that would be
safe for any other type of metadata that references symbols (not sure
there is anything other that debug metadata in that category).

> The debug info *shouldn't* be causing anything to be linked in that isn't
> already (since debug info should not affect code generation).  Instead,
> you can just have the `variable:` field point at nullptr.
>
>> With http://reviews.llvm.org/D14195 it was just leaving a decl. This caused LTO bootstrapping issues for global values in comdats which can't be decls (and aliases - which requires a separate fix to turn those into non-alias decls). The question is what to do about these metadata references - I assume they need to reference something in the dest module.
>
> If nothing got linked in for the DIGlobalVariable to point at, then I
> think it's correct to just point at `nullptr`.

Ok, will give that a try.

Thanks

>
>> Eventually with a follow on patch I have support for not linking in DISubprogram metadata that isn't needed. We would need to do the same for references to unmaterialized variables from DIVariable (any other metadata that can reference GVs?). These would have to be done along with the patch to move the metadata linking. For now, maybe just strip the decls from comdats in this case as well, but this assumes that the unreferenced global values and associated metadata will be removed by a later pass. Any other suggestions?
>



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413