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

Mon Aug 23 12:29:32 PDT 2021

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