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

[David Blaikie via llvm-commits]

On Thu, Nov 12, 2015 at 2:05 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Thu, Nov 12, 2015 at 1:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Thu, Nov 12, 2015 at 1:25 PM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>>
>>>
>>> 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.
>>>>
>>>
>>> That is not the case -- there is no guarantee that the comdat defs are
>>> available externally.
>>>
>>
>> What I'm wondering is if we can make that guarantee - since we can see
>> multiple instances of a comdat it seems like we might be able to benefit
>> from only producing the definition of them in one place by changing the
>> linkages.
>>
>
> Or waste a little compile time by always emitting comdats. Picking one
> file to emit comdat is probably doable -- but may cost compile time in the
> serial step of thinLTO which should be avoided.
>

I'm not sure I follow - you'd just pick the first one/any one, no? So far
as I can see it's just "there are N instances of this comdat, pick one at
random and promote linkonce_odr -> weak_odr, convert the rest to
available_externally" (or don't convert the rest, but only make newly
imported versions available externally if that's a compile time cost to
revisit the existing instances of the comdat)

- Dave


>
> David
>
>
>
>>
>>
>> - David
>>
>>
>>>
>>> David
>>>
>>>
>>>>
>>>> I guess I'm missing something - is there a pointer to when/why this is
>>>> necessary?
>>>>
>>>> (& 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)
>>>>
>>>>
>>>>>
>>>>> 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
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151112/6abbc3a3/attachment.html>