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

Fri Aug 20 20:55:32 PDT 2021

YolandaCY added a comment.

In D107034#2958541 <https://reviews.llvm.org/D107034#2958541>, @xur wrote:

> Sorry that I did not follow this patch closely. But after reading this I don't think this is a proper fix.
>
> Are you sure IRPGOFlag is dropped? CSPGO won't work properly if this happens. You might work-around the linker issue (unresolved symbols), but the resulted profile will not be marked properly.
> But I really doubt IRPGOFlag is indeed dropped. From the explanation of the example test case, it seems you just see the unsats from value profiling variables. Do you have other evidence that IRPGOFlag variable is dropped?

This variable is not dropped for the final binary, it’s just dropped for each module (if non-prevailing) during LTO/ThinLTO as there’s no uses or references. In the final instrumented binary, the variable is still kept. The example in discussion was trying to explain why it will introduce a unresolved symbol error, can be a seperate issue.

> A test case is really helping here (or the build log).

For the loss of IRPGOFlag in LTO should be easy to reproduce. (Just hard for the linker issue)
You may have a try to the test I added `lto_cspgo_gen.ll`, if exaimine output of the last command "llvm-dis %t.0.0.preopt.bc -o - ", you will find the variable "__llvm_profile_raw_version" is not emitted.

> Did you try MasRay's patch
> https://reviews.llvm.org/D108432
> to see if that fixes the problem?

MaskRay's patch may help avoid the linker error, but it wont fix the IRPGOFlag missing issue.

I actually have an alternative patch in a previous diff <https://reviews.llvm.org/D107034?vs=on&id=363777#toc>, if we have concern on using EnableValueProfiling flag.
Not sure if this looks better?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107034/new/

https://reviews.llvm.org/D107034