[PATCH] D14623: [ThinLTO] Comdat importing fixes and related cleanup

[Teresa Johnson via llvm-commits]

On Thu, Nov 12, 2015 at 12:56 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Thu, Nov 12, 2015 at 11:41 AM, Teresa Johnson via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>>
>> tejohnson created this revision.
>> tejohnson added reviewers: rafael, dexonsmith.
>> tejohnson added subscribers: llvm-commits, davidxl, joker.eph.
>>
>> A number of comdat related fixes, cleanup and test cases.
>>
>> I made a number of changes to get importing working when there are
>> comdats, with lots of new test cases involving comdats of various
>> selection types, linkage types, with and without aliases.
>>
>> The changes ensure we link in the full comdat group containing the
>> function indicated for importing. Previously I intended to require
>> that the importer specify the additional comdat group members for
>> importing on successive importing requests. However, in order to get the
>> comdat selection type correct it is significantly simpler to import the
>> full comdat group in a single pass.
>
>
> I thought all imported definitions were marked available_externally - in
> which case I'm not sure why it would be necessary to bring in the whole
> comdat group, since the comdat would never be emitted here.

Not always, e.g. comdat containing linkonce values.

Even in the case of those brought in available_externally, my concern
is when the comdat group also exists in the dest module (the one we
are importing into), but we want to select the one from the source
module (containing the imported function) because of the comdat
selection type. I am concerned about importing part of the comdat
group then eliminating it, leaving an incomplete group. Presumably the
linker will select the complete group from the right .o file in the
final link, but it seems wonky to me.

>
> I guess I'm missing something - is there a pointer to when/why this is
> necessary?

See the ThinLTO symbol linkage RFC here:
https://groups.google.com/d/msg/llvm-dev/-6H4GLv2BJw/77DBR-VjuHkJ.
There is a summary table at the end, but there are a lot of details.
It has changed a little bit based on Duncan's feedback and other
things found during implementation, but it is mostly accurate. See
also ModuleLinker::getLinkage() in LinkModules.cpp for the implemented
mapping.

>
> (& in fact, it might be useful to have this pass make one instance of a
> comdat group into an external definition (I guess that's probably not
> possible unless you have true whole program knowledge - maybe there's
> another linkage type which would produce the comdat group even if all the
> calls are optimized away) - and mark all the other instances
> available_externally? even potentially stripping the comdat groups from
> modules where the call sites are really cold)

I think this is not possible without whole program analysis as you mentioned.

Thanks,
Teresa

>
>>
>>
>> Additionally, if the module linker selects the source version of a
>> comdat not containing the imported function, if it is referenced
>> by an imported function we also import all its comdat group member
>> definitions (via the lazy linker) in the same importing pass.
>> In order to support this, we are now much more aggressive about lazy
>> linking global values during importing, which only need to be
>> linked in (as either declarations or definitions) if referenced by an
>> imported function.
>>
>> Also, because we may now import additional function bodies lazily, it is
>> no longer possible to determine up front whether an alias's aliasee is
>> going to be imported as a definition or not when we copy its prototype.
>> Therefore, we now convert aliases into declarations after all lazy
>> linking is complete, if the aliasee definition was not imported.
>>
>> Finally, since comdat members may not be declarations (including
>> available_externally), after linking is complete those that were
>> imported as linker declarations will be removed from comdats.
>>
>> http://reviews.llvm.org/D14623
>>
>> Files:
>>   lib/Linker/LinkModules.cpp
>>   test/Linker/Inputs/funcimport_comdat.ll
>>   test/Linker/Inputs/funcimport_comdat2.ll
>>   test/Linker/Inputs/funcimport_comdat3.ll
>>   test/Linker/funcimport.ll
>>   test/Linker/funcimport_comdat.ll
>>   test/Linker/funcimport_comdat2.ll
>>   test/Linker/funcimport_comdat3.ll
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>



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