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

[Petr Hosek via llvm-commits]

Correct me if I'm wrong, but I assumed we could use the same approach that
was suggested in https://bugs.llvm.org/show_bug.cgi?id=49155 for
`__llvm_prf_nm`.

That is, instead of emitting a single per-TU symbol with all the names, we
would emit one symbol for each name, all having the same section name. When
lowering, all those symbols get combined into a `SHF_MERGE & SHF_STRINGS`
section. We could then put each symbol into a group with the other
per-function symbols.

I tried this with coverage mapping locally and it's working well. The
problem is with COFF and Mach-O since they AFAIK don't have an equivalent
of SHF_COMPRESSED so we would lose the benefit of compression there.

We could continue using the current scheme for COFF and Mach-O and only use
the proposed approach (that is string merging with compression handled
transparently by the backend & the linker) for ELF, but it'd mean more
divergence between the formats which may be undesirable.

On Mon, Mar 1, 2021 at 1:26 PM Xinliang David Li <davidxl at google.com> wrote:

>
>
> 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/82c26a5a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3996 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210301/82c26a5a/attachment.bin>