[PATCH] D107034: [CSPGO] Set module flag for CSPGO to avoid IRPGO value loss in LTO

[Rong Xu via llvm-commits]

I sent my version of patch as https://reviews.llvm.org/D108581

On Mon, Aug 23, 2021 at 12:29 PM Rong Xu via Phabricator <
reviews at reviews.llvm.org> wrote:

> xur added a comment.
>
> In D107034#2959896 <https://reviews.llvm.org/D107034#2959896>, @YolandaCY
> wrote:
>
> > @xur This is also reproducible using the existing test case
> `PGOProfile/thinlto_cspgo_gen.ll`. If evaluating the generated
> "`%t.2.4.opt.bc`" containing non-prevailing IRPGO flag, you will find the
> value is missed. (The value is still reserved in "`%t.2.3.import.bc`".)
> >
> > In the LTO case, the symbol is dropped at *.preopt phase.
> >
> > In summary after tracing:
> >
> > 1. In LTO, the value is dropped when `addRegularLTO` as it has external
> linkage and non-prevailing.
> > 2. In ThinLTO, the value is first changed to `available_external`
> linkage by `thinLTOResolvePrevailingInModule` and
> `thinLTOResolvePrevailingGUID`. It is dropped later in the backend
> `GlobleOpt` pass by `deleteIfDead`.
> >
> > Hence to keep the value we may need to identify the IRPGO variable in
> all above places, make sure it is reserved, and then drop it after
> instrumentation to avoid duplicate symbols in later linkage. My feeling is
> that using a module flag may avoid too much hassle across multiple places
> and more complexity to the symbol resolution stuff. (as suggested by Reid)
> > Agree that mixing `EnableValueProfiling` with IRPGO flag is improper as
> it seems Clang also may support value profiling (from comment of
> `shouldRecordFunctionAddr` ).
> > Maybe we can rename the module flag to "`CSIRPGO`"? We can also remove
> the flag after instrumentation as no need for later passes.
>
> OK. I now see the problem.
>
> I added IRPGOFlag was to only used to mark the profile as IR
> instrumentation (rather clang instrumentation).
> But it was later used to coordinate instrumentation pass and lowering
> pass.  This breaks in thin-lto and lto.
>
> IRPGOFlag now not only is used in enabling value profiling, but also in
> comdat renaming.
>
> I still don't want to use module flag -- using IRPGOFlag still preferred.
>
> I will fix this.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D107034/new/
>
> https://reviews.llvm.org/D107034
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210823/c3f7b1da/attachment.html>