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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D107034#2954190 <https://reviews.llvm.org/D107034#2954190>, @YolandaCY wrote:

> 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.

OK, clang emitted weak_odr directly (I am not familiar when weak_odr is used on Windows).

>> 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

Is `__profd_assign` referenced by a non-assign function? The `__profd_assign` reference is a result of inlining.

>>> 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.

`DoComdatRenaming` is PGOInstrumentation.cpp's comdat renaming.
`canRenameComdatFunc` is InstrProfiling.cpp's comdat renaming. It is not disabled.

See the test `indirect_call_profile.ll`: the renaming still happens.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107034