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

[Yolanda via Phabricator via llvm-commits]

YolandaCY added a comment.

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

> So, this appears to be a PE/COFF specific problem, due to our wrestling with link.exe's comdat limitation....
>
> I am still unclear yet whether this is only specific to post-inlining instrumentation. If yes, this is a CSPGO specific problem. Perhaps we can think about dropping link.exe compatibility if this turns out to be too much of a hassle...
>
> ---
>
> I do not understand this part:
>
>> Whereas the symbol from thin module has external attribute, and is associated to __profc_assign* who is non-prevailing and should not be inserted. While there is a prevailing __profc_assign* existing in symbol table, there is no __profd_assign*, thus reported as unresolved symbol.
>
> Summary of the issue:
>
> - The final (after implicit or distributed ThinLTO generated object files) linker command line looks like `lld-link ... regular.o thin.o`.
> - `regular.o` (regular LTO) emits `__prof[cd]_assign`. `__profc_assign` as the comdat leader is prevailing. profc is linkonce_odr while profd is private.
> - `thin.o` (Thin LTO) emits `__prof[cd]_assign`. `__profc_assign` as the comdat leader is non-prevailing. Both profc and profd are linkonce_odr.

In `thin.o`, both profc and profd are weak_odr linkage.

> Does your change make `regular.o __profc_assign` linkonce_odr?

Yes. Just make sure the ValueProfiling flag available and will keep it as it is for COFF.

> Which variable in a non-prevailing comdat is referenced, causing an undefined symbol error?

It's __profd_assign. As its associate parent (__profc_assign) is non-prevailing, it will get discarded as the parent. See https://github.com/llvm/llvm-project/blob/main/lld/COFF/InputFiles.cpp#L349

>> I think by correcting the profile var name with a hash suffix when IRPGO enabled should avoid such inconsistency in different translation units. And the linker error helped exposing the issue.
>
> Any idea why a hash suffix isn't used?
> See `canRenameComdatFunc`, is the address of `assign` taken?

The `canRenameComdatFunc` is turned off by default by flag DoComdatRenaming <https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp#L188>. I can have a try later.
The `getVarName` for profile variables will add FunHash <https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp#L766> as suffix in this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107034