[PATCH] D106228: [LTO] Add SelectionKind to IRSymtab and use it in ld.lld/LLVMgold

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D106228#2888164 <https://reviews.llvm.org/D106228#2888164>, @MaskRay wrote:

> In D106228#2887867 <https://reviews.llvm.org/D106228#2887867>, @tejohnson wrote:
>
>> In D106228#2887793 <https://reviews.llvm.org/D106228#2887793>, @MaskRay wrote:
>>
>>> In D106228#2887760 <https://reviews.llvm.org/D106228#2887760>, @sbc100 wrote:
>>>
>>>> In D106228#2887667 <https://reviews.llvm.org/D106228#2887667>, @MaskRay wrote:
>>>>
>>>>> In D106228#2887624 <https://reviews.llvm.org/D106228#2887624>, @tejohnson wrote:
>>>>>
>>>>>> I'm not sure I understand the comdat noduplicates selection kind. The lang ref says it means "The linker requires that only section with this COMDAT key exist.". Does that mean that we should never see more than one section with that COMDAT key, in which case it seems like we shouldn't have the case you describe (2 noduplicates comdat with the key "__profc_foo"), or does it mean something else?
>>>>>
>>>>> "The linker requires that only section with this COMDAT key exist." The sentence looks unclear.
>>
>> Yeah. At a minimum, I think a word is missing (should this be "only *one* section ..." ?). But it still doesn't describe the semantics well.
>> [...]
>>
>> The name and description unfortunately don't imply this to me at least. Would you be able to update the langref (in a separate patch) to update the description so it is clearer?
>
> Sounds good! Sent D106300 <https://reviews.llvm.org/D106300> to clarify noduplicates and misc other things.
>
>> Also, what happens in regular LTO when IR modules with two "noduplicates" comdats with the same key are merged? Is the assumption that the symbols within them would have been deduplicated after symbol resolution, so that we don't end up violating your (1) above?
>
> The LTO behavior needs to match the linker. I think LTO needs to retain all members and place all of them into the comdat.

I assume that any weak symbols of the same name in those comdats would be deduplicated though? For regular LTO what would the resulting comdat group look like in the merged IR? What if the __profc_foo was weak in both modules being linked?

> For a case like merging two comdats of different selection kinds, I think the compiler may have to issue an error.





================
Comment at: lld/test/ELF/lto/comdat-noduplicates.ll:18
+
+; IR: gv: (name: "__profd_foo", {{.*}} guid = [[PROFD:[0-9]+]]
+; IR: gv: (name: "foo", {{.*}} guid = [[FOO:[0-9]+]]
----------------
I think it would be good to test a few more cases: for non-distributed (no --thinlto-index-only) check the resulting symbols/comdats in the IR dumped by save-temps; test regular LTO as well, since that is presumably affected by this change too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106228