[PATCH] D97585: [InstrProfiling] Use llvm.compiler.used instead of llvm.used for ELF

[Xinliang David Li via llvm-commits]

On Mon, Mar 1, 2021 at 12:26 PM Fāng-ruì Sòng <maskray at google.com> wrote:

> On Mon, Mar 1, 2021 at 12:06 PM Xinliang David Li <davidxl at google.com>
> wrote:
>
>>
>>
>> On Mon, Mar 1, 2021 at 11:53 AM Fangrui Song via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> MaskRay added a comment.
>>>
>>> In D97585#2595131 <https://reviews.llvm.org/D97585#2595131>, @rnk wrote:
>>>
>>> > Wait, are you telling me that `__profd/__profc` data can participate
>>> in linker GC, and that is not a problem?
>>>
>>> Yes on ld.lld, since D96636 <https://reviews.llvm.org/D96636> .  The GC
>>> is more useful with D96757 <https://reviews.llvm.org/D96757>, which
>>> moved metadata sections which were not within a `comdat any` into a `comdat
>>> noduplicates` group.
>>>
>>> > I have actually had to optimize the processing of `/INCLUDE:`
>>> directives for COFF (D78845 <https://reviews.llvm.org/D78845>), because
>>> I assumed that it was critical that `__profd` be treated as a GC root.
>>>
>>> Yes. I think getting rid of the local linkage limitation is important
>>> for COFF GC.
>>>
>>> > But, on ELF, they were never GC roots, and coverage/PGO has worked
>>> just fine there.
>>>
>>> Yes. For some sections (`__llvm_prf_nm` `__llvm_prf_vnds`), they work
>>> just because linkers have such a rule:
>>>
>>> > __start_/__stop_ references from a live input section retains all
>>> non-SHF_LINK_ORDER non-SHF_GROUP C identifier name sections.
>>>
>>> `-z start-stop-gc` can drop the rule, so we need D97649 <
>>> https://reviews.llvm.org/D97649> for robustness.
>>>
>>> That said, I am still not very confident with GCness for the compressed
>>> `__llvm_prf_nm`. I think for PGO it is optional, so whether we retain or
>>> discard it probably does not matter much.
>>
>>
>> That is not correct -- names are important for symbol resolution for
>> indirect call promotion purposes in PGO.
>>
>> David
>>
>
> Thanks for the clarification. There can be multiple
> `__llvm_prf_{cnts,data,vals}` but one monolithic `__llvm_prf_nm` in an
> object file.
> Does the content of __llvm_prf_nm need changes when
> some `__llvm_prf_{cnts,data,vals}` are discarded? If not, the current
> scheme is working.
>

Ideally yes, but it should work without updating as it just contains a
superset of symbol names we care about.

David

>
>
>>
>>
>>
>>> For coverage mapping it might be a problem.
>>> But this patch and D97649 <https://reviews.llvm.org/D97649> do not
>>> change the status quo.
>>>
>>> > So, really, all along, llvm.used was only being used to block LLVM IPO
>>> transforms like globalopt, not to establish a GC root. And, as you point
>>> out, we don't get the GC root behavior for internal `__profd/__profc`
>>> globals, and coverage/PGO seems to work fine.
>>>
>>>
>>>
>>> > What about MachO? Should all platforms not treat __profd as a GC root?
>>>
>>> Since MachO does not have comdat. I think it has to be conservative and
>>> treats every metadata section GC roots (`llvm.used`).
>>>
>>> All binary formats with ELF section group compatible semantics ("if
>>> section A in a section group is retained, all members in the group are
>>> retained as well") should be able to use `llvm.compiler.used`.
>>> I believe COFF can do it, but as you said, `/INCLUDE:` needs fixes to
>>> support local linkage `GlobalObject`s.
>>>
>>>
>>> Repository:
>>>   rG LLVM Github Monorepo
>>>
>>> CHANGES SINCE LAST ACTION
>>>   https://reviews.llvm.org/D97585/new/
>>>
>>> https://reviews.llvm.org/D97585
>>>
>>>
>
> --
> 宋方睿
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210301/3b4028a0/attachment.html>