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

[Fāng-ruì Sòng via llvm-commits]

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.


>
>
>
>> 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/effe9439/attachment.html>