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

Mon Jul 19 12:30:34 PDT 2021

MaskRay added a comment.

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.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106228