[PATCH] D80765: [ELF] Handle bitcode comdat groups separately to deduplicate thinlto comdat sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D80765#2174250 <https://reviews.llvm.org/D80765#2174250>, @dblaikie wrote:

> In D80765#2174141 <https://reviews.llvm.org/D80765#2174141>, @hoyFB wrote:
>
> > In D80765#2170820 <https://reviews.llvm.org/D80765#2170820>, @dblaikie wrote:
> >
> > > Yeah - one alternative here would be for ThinLTO To "home" debug type information, the same way I believe it "homes" inline functions. (original/incoming IR to the thinlink may have multiple definitions of inline functions - one in every module that has a call to the inline function - but something in the think link summary basically says "drop this definition" in all but one of those modules and in that one blessed module it says "always produce a weak definition of this function, even if you don't need it" - something like that should be done with type information, which would remove the redundancy in the object files)
> > >
> > > In the case of a full executable link (with the homing above implemented), I'd then suggest not enabling type units, since they just add overhead and wouldn't reduce any duplication - in the case of a static library built with ThinLTO (if that's even a thing), then you'd still potentially want to use type units so they could be deduplicated against other libraries.
> >
> >
> > Enabling compile-time type deduplication is indeed an ideal solution, as you pointed out finding a prevailing symbol definition and dropping others for duplicate functions from different modules. This would require quite some work in parsing type metadata on the IR and might slow down thinLink.
>
>
> Fair point - though presumably the first stage compile would do the metadata walk through the IR, provide a list of types in the thin summary file and the thin link would then communicate to the backend compiles specifying which types each backend compile should emit into its respective object file.
>
> >   Also this may not completely address deduplication of new data introduced during LTO postLink code generation time. The type unit level deduplication could be the last resort to handle the duplicates, like what is done in non-LTO build.
>
> Not sure I follow - what duplication would remain if the thin link homed type descriptions? When linking to non-LTO (normal machine code/object files) libraries? Yeah, fair point (tangential warning: LLVM's type unit hashes are non-DWARF-conforming and not compatible with GCC's type unit hashes... :/)
>
> It /sounds/ like, maybe the original fix in D62884 <https://reviews.llvm.org/D62884> maybe was going in the wrong direction, and instead ThinLTO should be removing the comdat group from "homed" inline functions? Then there wouldn't be a need for a special case in how comdats are handled.


I am with @dblaikie here. A bit of archaeology finds that rG4de44b7ef87bcef83798eee69fdcbfab9866d52e <https://reviews.llvm.org/rG4de44b7ef87bcef83798eee69fdcbfab9866d52e> was probably going in the wrong direction but it was indeed the simplest/least intrusive approach.
D62884 <https://reviews.llvm.org/D62884> just made some refactorings. Just a thought: for ThinLTO backend, dropping comdat in processGlobalsForThinLTO may achieve some results. However, one issue is that on the MC layer the section group signature is part of ELFSectionKey.

  .section foo,"aG", at progbits,aaa,comdat
  .section foo,"aG", at progbits,bbb,comdat

are two sections. Dropping the comdat key will make them one combined section. Unique linkage may work around it but there may be a fair amount of complexity in TargetLoweringObjectFileImpl/MC.
I agree with pcc that LTO should not generate additional arbitrary comdat objects.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80765/new/

https://reviews.llvm.org/D80765