Page MenuHomePhabricator

[ELF] Handle bitcode comdat groups separately to deduplicate thinlto comdat sections
Needs ReviewPublic

Authored by christylee on May 28 2020, 3:29 PM.

Details

Summary

This change allows lld to deduplicate bitcode file comdat groups, which will allow it to deduplicate .debug_types sections in thinlto.

For more context, see https://reviews.llvm.org/D62884

Diff Detail

Event Timeline

christylee created this revision.May 28 2020, 3:29 PM
christylee edited the summary of this revision. (Show Details)May 28 2020, 3:34 PM
christylee added reviewers: ruiu, sbc100, dblaikie.
christylee added subscribers: wenlei, hoy.

(I'm probably not the right person to do detailed/semantic review of this, but here's some basic things I spotted)

lld/ELF/InputFiles.cpp
609–617

typo? ("proceed" was intended to be "processed", perhaps?)

1568–1571

Unrelated change in this patch - probably best to remove it or commit it separately?

christylee marked an inline comment as done.

Fixed typo and removed unrelated change.

christylee marked an inline comment as done.May 28 2020, 4:29 PM

Would you mind making the equivalent changes to lld/wasm?

And a test?

lld/ELF/InputFiles.cpp
393

Perhaps this would better be names something like isLTOOutput? Otherwise it sounds like it might a bitcode object which is a different class here.

This comment was removed by christylee.
christylee edited the summary of this revision. (Show Details)May 29 2020, 12:41 PM
christylee edited the summary of this revision. (Show Details)

@sbc100 What would be a good way to go about writing a test for this? The reason why I noticed this bug was because .debug_types are not deduplicated in thinlto, but writing a lld test case using debug metadata seems clunky.

I've also tried something like this but it doesn't repro:

; REQUIRES: x86
; RUN: llvm-as %s -o %t.o
; RUN: llc %t.o -o %t2.o -filetype=obj
; RUN: ld.lld %t.o %t2.o %t.o %t2.o -o %t3.o --shared
; RUN: llvm-readobj --symbols %t3.o | FileCheck %s

; CHECK:      Name: foo
; CHECK-NEXT: Value:
; CHECK-NEXT: Size: 1
; CHECK-NEXT: Binding: Global
; CHECK-NEXT: Type: Function
; CHECK-NEXT: Other: 0
; CHECK-NEXT: Section: .text

target triple = "x86_64-unknown-linux-gnu"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"

$foo = comdat any
define void @foo() comdat {
  ret void
}

@sbc100 What would be a good way to go about writing a test for this? The reason why I noticed this bug was because .debug_types are not deduplicated in thinlto, but writing a lld test case using debug metadata seems clunky.

I've also tried something like this but it doesn't repro:

; REQUIRES: x86
; RUN: llvm-as %s -o %t.o
; RUN: llc %t.o -o %t2.o -filetype=obj
; RUN: ld.lld %t.o %t2.o %t.o %t2.o -o %t3.o --shared
; RUN: llvm-readobj --symbols %t3.o | FileCheck %s

; CHECK:      Name: foo
; CHECK-NEXT: Value:
; CHECK-NEXT: Size: 1
; CHECK-NEXT: Binding: Global
; CHECK-NEXT: Type: Function
; CHECK-NEXT: Other: 0
; CHECK-NEXT: Section: .text

target triple = "x86_64-unknown-linux-gnu"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"

$foo = comdat any
define void @foo() comdat {
  ret void
}

I'm sorry I've not looked into this in much detail but shouldn't this same bug reproduce with all comdats? Maybe there is something different about debug metatdata? BTW, the test above doesn't use lto or thinglto does it?

christylee updated this revision to Diff 267420.EditedMay 29 2020, 5:01 PM

Addressed comments:

1). Changed isBitcodeFile to isLTOOutput.
2). Added test. Before this patch, lld does not deduplicate comdat sections generated by the backend. .debug_types is an example.

Change for COFF to follow.

Changed variable name bitcodeComdatGroup to ltoOutputComdatGroup for consistency.

Fixed a comment.

We don't need to make the same change to COFF. COFF does not ignore comdats, it attempts to add a comdat symbol as long as the symbol is external. It also tracks whether a symbol is from a regular object file or bitcode file so it won't have duplicates.

christylee retitled this revision from [lld] Handle bitcode comdat groups separately to deduplicate thinlto comdat sections to [ELF] Handle bitcode comdat groups separately to deduplicate thinlto comdat sections.Jun 3 2020, 12:14 PM
Harbormaster completed remote builds in B58974: Diff 268275.
hoyFB added a reviewer: grimar.Jun 8 2020, 1:55 PM
grimar added inline comments.Jun 9 2020, 1:42 AM
lld/ELF/SymbolTable.h
66

Double space before "Define".

67

fromt -> from

68

Could you expand the comment to mention why it is important to distinct them?

christylee updated this revision to Diff 269693.Jun 9 2020, 4:05 PM
christylee marked an inline comment as done.

Fixed comment.

The LLD change looks fine to me. But I am not well familiar with LTO stuff and hence do not feel myself is a right person to approve it.

@grimar Do you know who might be a good reviewer?

@grimar Do you know who might be a good reviewer?

@tejohnson is a good one.

I'll also get to this patch soon. I tried an earlier version of this patch and noticed issues testing internally. I'll retest since the patch has changed a lot.

@grimar Do you know who might be a good reviewer?

@tejohnson is a good one.

Since this is very specific to lld, and not in the LLVM LTO handling, I'm not sure I'm the best person to review. I looked at the change but don't have enough understanding of how lld is otherwise handling comdat groups to do a really informed review.

I'll also get to this patch soon. I tried an earlier version of this patch and noticed issues testing internally. I'll retest since the patch has changed a lot.

Sounds good.

lld/test/ELF/lto/debug-types-deduplication.ll
4

IMO it is better to test ThinLTO by using "opt -module-summary" to create the bitcode object, and not have the serialized summary. It makes it clearer that it is a ThinLTO test.

MaskRay added a subscriber: pcc.Jun 17 2020, 10:48 PM

Apologies for my slowness getting to this patch.

symtab->ltoOutputComdatGroups does work:

% readelf -g a3.o.lto.o a3.o1.lto.o 

File: a3.o.lto.o

COMDAT group section [    4] `.group' [4068369915778327548] contains 2 sections:
   [Index]    Name
   [    5]   .debug_types
   [    6]   .rela.debug_types

File: a3.o1.lto.o

COMDAT group section [    4] `.group' [4068369915778327548] contains 2 sections:
   [Index]    Name
   [    5]   .debug_types
   [    6]   .rela.debug_types

The .debug_types from a3.o1.lto.o can be discarded by the new COMDAT logic. However, I am concerned that making it general as this patch does can miss some codegen bugs. See @pcc's argument in
in https://reviews.llvm.org/D56015#1339411 . Since .debug_types (notably, a non-SHF_ALLOC section) is the only COMDAT rule this patch will discard, how about special casing .debug_types (i.e. if isLTOOutput && the group is related to .debug_types)?

MaskRay added inline comments.Jun 17 2020, 10:51 PM
lld/test/ELF/lto/debug-types-deduplication.ll
37

Can you drop some unneeded metadata nodes here?

hoyFB added a subscriber: hoyFB.Jun 17 2020, 11:48 PM

Apologies for my slowness getting to this patch.

symtab->ltoOutputComdatGroups does work:

% readelf -g a3.o.lto.o a3.o1.lto.o 

File: a3.o.lto.o

COMDAT group section [    4] `.group' [4068369915778327548] contains 2 sections:
   [Index]    Name
   [    5]   .debug_types
   [    6]   .rela.debug_types

File: a3.o1.lto.o

COMDAT group section [    4] `.group' [4068369915778327548] contains 2 sections:
   [Index]    Name
   [    5]   .debug_types
   [    6]   .rela.debug_types

The .debug_types from a3.o1.lto.o can be discarded by the new COMDAT logic. However, I am concerned that making it general as this patch does can miss some codegen bugs. See @pcc's argument in
in https://reviews.llvm.org/D56015#1339411 . Since .debug_types (notably, a non-SHF_ALLOC section) is the only COMDAT rule this patch will discard, how about special casing .debug_types (i.e. if isLTOOutput && the group is related to .debug_types)?

Thanks for taking time reviewing this change. It is an interesting argument whether LTO should introduce new COMDAT symbols. Do we have a conclusion on this? So far the .debug_types are the only COMDAT we've seen introduced by LTO, but in theory any LTO-exclusive pass may introduce a new COMDAT symbol and it's up to the pass how to set the symbol's linkage type.

christylee marked an inline comment as done.Jun 24 2020, 9:34 AM
christylee added inline comments.
lld/ELF/InputFiles.cpp
611

@MaskRay

Since .debug_types (notably, a non-SHF_ALLOC section) is the only COMDAT rule this patch will discard, how about special casing .debug_types (i.e. if isLTOOutput && the group is related to .debug_types)?

At the time we run this check, not all InputSections have been initialized, so a the group might be related to an uninitialized section. Since it might be uninitialized, what's the best way to check if that section is non-SHF_ALLOC?

MaskRay added inline comments.Jun 24 2020, 10:07 AM
lld/ELF/InputFiles.cpp
611

This is difficult. An alternative is to use ltoOutputComdatGroups but assert that no collision happens

if (isLTOOutput) {
  CachedHashStringRef ref(signature);
  isNew = symtab->ltoOutputComdatGroups.try_emplace(ref, this).second);
  if (symtab->comdatGroups.coumt(ref))
    errorOrWarn(toString(this) + ": LTO output should not emit a section group whose signature collides with a non-LTO group signature");
}
christylee marked an inline comment as done.Jun 24 2020, 11:10 AM
christylee added inline comments.
lld/ELF/InputFiles.cpp
611

When we link something with thinlto, we first process the comdat groups when they are in bitcode form. We add that to symtab->comdatGroups since we want symtab->comdatGroups to contain all possible comdatGroups. After compiling the bitcode to object files, we process comdat groups again. At this point, if they are lto output, then we need to check whether we've seen it from other lto outputs. In other words, if something is in symtab->ltoOutputComdatGroups, then it must also be in symtab->comdatGroups, but we need the two distinct maps to ensure there are no duplicate symbols.

christylee marked 2 inline comments as done and an inline comment as not done.Thu, Jul 23, 9:36 AM
christylee added inline comments.
lld/ELF/InputFiles.cpp
611

@MaskRay , can we ship this as is?

lld/test/ELF/lto/debug-types-deduplication.ll
37

I've tried dropping the metadata but this is as small as I can get it.

In case we are concerned about correctness, we've been using this patch internally for a couple of months with no problems.

@MaskRay , can we ship this as is?

This still looks like a workaround done at an inappropriate layer to me. The .debug_types code generator should probably know that some .debug_types sections should not be emitted.
BTW, do you have statistics about the effectiveness for this change?

BTW, do you have statistics about the effectiveness for this change?

@MaskRay , one of our thinlto binaries went from a 6.9 GiB .debug_types section to 420 MiB, while another went from 7.8 GiB to 370 MiB.

dblaikie added a comment.EditedThu, Jul 23, 2:59 PM

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.

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. 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.

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 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.

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 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 was probably going in the wrong direction but it was indeed the simplest/least intrusive approach.
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",@progbits,aaa,comdat
.section foo,"aG",@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.

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 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 was probably going in the wrong direction but it was indeed the simplest/least intrusive approach.
D62884 just made some refactorings. Just a thought: for ThinLTO backend, dropping comdat in processGlobalsForThinLTO may achieve some results.

Yeah, maybe something near/similar to https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Utils/FunctionImportUtils.cpp#L275 - where the comdat is imported as available externally, and its comdat group is stripped. Theory is, maybe we should be stripping the comdat on the other side too.

Hmm, maybe not, though? What about non-whole-program-ThinLTO? There could be a comdat copy of this function in a static library that it should be deduplicated against?

I take it the underlying assumption lld is making is that a comdat group is only kept alive by references from within the object file that defines it? Is that formally specified/required? If not, then maybe we could/should remoev that assumption, so that ThinLTO can work naturally with non-(Thin)LTO archives and still do the suitable comdat deduplication or dropping?

However, one issue is that on the MC layer the section group signature is part of ELFSectionKey.

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

are two sections. Dropping the comdat key will make them one combined section.

Is that a problem? When linked they end up in the same section, right?

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.

What additional arbitrary comdat objects are being generated or proposed to be generated?

hoyFB added a comment.Mon, Jul 27, 3:42 PM

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 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 was probably going in the wrong direction but it was indeed the simplest/least intrusive approach.
D62884 just made some refactorings. Just a thought: for ThinLTO backend, dropping comdat in processGlobalsForThinLTO may achieve some results.

Yeah, maybe something near/similar to https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Utils/FunctionImportUtils.cpp#L275 - where the comdat is imported as available externally, and its comdat group is stripped. Theory is, maybe we should be stripping the comdat on the other side too.

Hmm, maybe not, though? What about non-whole-program-ThinLTO? There could be a comdat copy of this function in a static library that it should be deduplicated against?

This is probably fine if the static library is a native input to lld , since lld itself can deduplicate comdat groups of its native input.

I take it the underlying assumption lld is making is that a comdat group is only kept alive by references from within the object file that defines it? Is that formally specified/required? If not, then maybe we could/should remoev that assumption, so that ThinLTO can work naturally with non-(Thin)LTO archives and still do the suitable comdat deduplication or dropping?

However, one issue is that on the MC layer the section group signature is part of ELFSectionKey.

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

are two sections. Dropping the comdat key will make them one combined section.

Is that a problem? When linked they end up in the same section, right?

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.

What additional arbitrary comdat objects are being generated or proposed to be generated?

So far the Dwarf types are the only new artifacts generated by the LTO backend. What I was wondering was new LTO-exclusive codegen passes that may introduce new comdat groups, but sound like it is a design principle that new symbols/data should be only generated in preLink LTO phase.

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 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 was probably going in the wrong direction but it was indeed the simplest/least intrusive approach.
D62884 just made some refactorings. Just a thought: for ThinLTO backend, dropping comdat in processGlobalsForThinLTO may achieve some results.

Yeah, maybe something near/similar to https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Utils/FunctionImportUtils.cpp#L275 - where the comdat is imported as available externally, and its comdat group is stripped. Theory is, maybe we should be stripping the comdat on the other side too.

Hmm, maybe not, though? What about non-whole-program-ThinLTO? There could be a comdat copy of this function in a static library that it should be deduplicated against?

This is probably fine if the static library is a native input to lld , since lld itself can deduplicate comdat groups of its native input.

Oh, yeah, fair point.

I take it the underlying assumption lld is making is that a comdat group is only kept alive by references from within the object file that defines it? Is that formally specified/required? If not, then maybe we could/should remoev that assumption, so that ThinLTO can work naturally with non-(Thin)LTO archives and still do the suitable comdat deduplication or dropping?

However, one issue is that on the MC layer the section group signature is part of ELFSectionKey.

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

are two sections. Dropping the comdat key will make them one combined section.

Is that a problem? When linked they end up in the same section, right?

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.

What additional arbitrary comdat objects are being generated or proposed to be generated?

So far the Dwarf types are the only new artifacts generated by the LTO backend.

Ah, OK, "new" in the sense that they weren't present as comdat groups in the ThinLTO input IR.

What do we gain by having this restriction? I guess it removes a certain amount of work the linker would otherwise do when performing the final link step in ThinLTO?

Anyone have a sense of the significance of that restriction compared to linking "as normal"?