[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 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



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