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

[Yolanda via Phabricator via llvm-commits]

YolandaCY marked 2 inline comments as done.
YolandaCY added a comment.

In D107034#2944459 <https://reviews.llvm.org/D107034#2944459>, @MaskRay wrote:

>> The IRPGOFlag symbol (__llvm_profile_raw_version) will be dropped when identified as non-prevailing for either regular or thin LTO during the mixed-LTO mode compilation. This caused an unresolved symbol linker error when enable CSPGO over ThinLTO after https://reviews.llvm.org/D103717.
>
> I am concerned that `EnableValueProfiling` might not be added to the correct place. (Why specifically to CSPGO?) Can you give a concrete example for the mixed LTO scenario?
> What's the "duplicate symbol" error?

The background is when enable CSPGO over ThinLTO, it requires a seperate pass to create IRPGO flag in prelink, hence the var will later be subject to symbol resolution during LTO. This is not the case for PGO though. See references at:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp#L1614
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Passes/PassBuilder.cpp#L1183

For mixed LTO, I think it should relate to flag "-fsplit-lto-unit" (also subject to the whole program devirtualization optimization pass). I included a test case for hybrid lto in a previous revision, see: https://reviews.llvm.org/D107034?vs=on&id=363666#toc
You may find a "...!"ThinLTO", i32 0}" mudule flag which indicate this file will be added to regular LTO, while the other without it will be added for ThinLTO.

Here are some more references on why mixed LTO is needed and how:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp#L34
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp#L269

The duplicate symbol error if when I simply try to reserve the IRPGO flag in regular LTO when it is identified as non-prevailing, I will get such error for ELF, but not COFF.

Hope this helps.



================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:1148
+
+    // The IRPGO flag var (__llvm_profile_raw_version) has external linkage,
+    // it may not be kept if it's non-prevailing in a mixed-LTO mode (both
----------------
MaskRay wrote:
> The way `comdat any` is used, it is unsupported if you mix non-CS and CS PGO object files. The first copy is picked by the linker.
> 
> This does not seem a proper fix to me. If you can give a concrete example, if may help me to understand.
Yes, I think mixing non-CS and CS PGO seems not a normal practice. As CSPGO is trying collect profile after inlining by applying profile created by PGO.
The user guide for CSPGO is run after PGO as described here:
https://clang.llvm.org/docs/UsersManual.html#cmdoption-fcs-profile-generate

Hope this makes sense now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107034