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

Thu Nov 12 14:02:56 PST 2015

On Thu, Nov 12, 2015 at 1:45 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Thu, Nov 12, 2015 at 1:21 PM, Teresa Johnson <tejohnson 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.
>>
>> 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.
>
>
> Sorry, I'm not quite following. My familiarity with comdats is perhaps
> limited - but mostly for linkonce situations "we put this in multiple object
> files, but we really only need one copy of the group in the final
> executable".
>
> I'm imagining an intermodule optimization where we promote the group in one
> file to "always emitted (not just only emitted if called/used - because this
> version will be relied upon by other modules now) but can still be
> deduplicated by the linker" (maybe we don't have a linkage type for that?
> Would weak suffice? From my reading of the LangRef it sounds like exactly
> the right thing) and then marking them in all the other modules we see as
> available_externally.
>
> In this way, the definition would be available for optimization, but we
> would avoid code generating duplicate entities only to have the linker
> deduplicate them later. (but, since we don't have whole program information
> - we'd still be able to deduplicate this single ThinLTO definition with any
> non-ThinLTO definitions at link time)

ThinLTO isn't whole program, and what you are describing would need
whole program analysis to determine which module will emit the comdat
into the object file and guarantee that it will be emitted somewhere
so that the other modules can drop it even if referencing the comdat
members. Weak linkage does what you mention (keep it even if
unreferenced, deduplicate in link), but like I said, ThinLTO doesn't
have the visibility to make those kind of changes.

>
>>
>>
>> >
>> > 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.
>
>
> Ah, thanks for the ref.
>
> "WeakODRLinkage symbols cannot be marked AvailableExternallyLinkage, because
> if the def is later dropped (by the EliminateAvailableExternally pass), the
> new decl is marked ExternalLinkage. For these weak symbols, however, the
> correct linkage for the decl is actually ExternalWeakLinkage, so that they
> get treated appropriately by the linker."
>
> That helps me understand some of it...
>
> *reads further*
>
> It's not called out, but I imagine there's the same problem with
> LinkonceODRLinkage? We can't take a linkonce_odr function, change it to
> weak_odr, import it into the other module as available_externally? Because
> once we drop the definition, we lose the "weak" attribute and would produce
> an incorrect reference?
>
> That seems unfortunate - it's mentioned in the weak_odr case that weak
> symbols are rare - linkonce_odr functions are probably /really/ common and
> might benefit from this. But it's purely a linker/object size optimization,
> so I could imagine it would be a valid optimization to defer until later?

WeakODR is one thing that changed since the proposal was sent (see
Duncan't review and my response). They are now imported as
available_externally defs (see comments in ModuleLinker::getLinkage
for details).

LinkOnceODR imported GVs stay LinkOnceODR. The difference between
WeakODR and LinkOnceODR is because the latter is discardable if
unused, so there is no guarantee that it will be emitted into the
object file in its original module (e.g. if all uses inlined). WeakODR
is not discardable if unused, so is guaranteed to be available.

>
>>
>>
>> >
>> > (& 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
>
>

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