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

Mon Jul 19 11:22:11 PDT 2021

tejohnson added a comment.

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 semantics are: (1) Within a module, a section can be the member of at most one ELF section group or PE/COFF comdat. (2) if two groups in two modules have the same key, they are not deduplicated.
>>>
>>> One `comdat noduplicates` may have multiple members. These members will be retained or discarded as a unit. The ability is used by instrumentation to keep some related sections as a unit.
>>>
>>> The failure scenario happened for a PGO+ThinLTO case, where two modules have `comdat noduplicates` of the same name. We should not perform deduplication.
>>
>> So it sounds like the meaning of "noduplicates" is "allow duplicates"  or maybe "do not de-duplicate" ?   Is that right?
>
> Right. Other selection kinds (`any` is the most common; exactmatch/largest/samesize are PE/COFF specific) use the key for duplication.
> For `noduplicates`, the key is just a key to keep several members in the same group; it is not overloaded with the deduplication semantics.

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?

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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106228