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

[YolandaCY]

Summary

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.

Try set a CSIRPGO flag for CSPGO to avoid this symbol resolution caused value loss.

[YolandaCY]

Use a new flag to avoid test failures

[YolandaCY]

Fix for CSPGO only

[YolandaCY]

cleanup

[YolandaCY]

lint fix

[YolandaCY]

rebase

[YolandaCY]

Could you help take a look? This is trying to address a linker error for CSPGO with ThinLTO on Windows after a recent CL to keep __profd_ variable private.

[rnk]

Should we set the "EnableValueProfiling" module flag as an alternative way to express that value profiling is enabled? That flag wouldn't be subject to this kind of symbol resolution.

[YolandaCY]

Thank you Reid for the comment. (Sorry I missed your comment last week.)

Yes, setting "EnableValueProfiling" flag can be an option. Just I encountered a test failure at "llvm/test/Instrumentation/InstrProfiling/comdat.ll" that will change the generated "__prof*" variable name with a trailing 0. The test may need be revised. I'm not quite sure if "EnableValueProfiling" flag can be equal to the IRFlag value, if there's no objection, I can try this method.

[YolandaCY]

Change to set EnableValueProfiling module flag

[YolandaCY]

cleanup unrelated changes

[rnk]

llvm/lib/LTO/LTO.cpp

1236 ↗

(On Diff #365957)

My only concern is that we have to do this in two places. Is there a better place we can do this? Maybe the frontend, or wherever we enable IR PGO? These feel like the wrong places.

[YolandaCY]

Set the flag earlier when generate the IRPGO var

[YolandaCY]

llvm/lib/LTO/LTO.cpp

1236 ↗

(On Diff #365957)

Thank you Reid for the suggestion. Agree that setting this ealier when we enable the IR PGO is better. Revised the patch accordingly.

[rnk]

lgtm

@MaskRay, please take a look.

[MaskRay]

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?

llvm/test/Transforms/PGOProfile/lto_cspgo_gen.ll

12

This test is unclear why EnableValueProfiling is set for CSPGO.

[MaskRay]

llvm/lib/ProfileData/InstrProf.cpp

1148

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.

[YolandaCY]

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

llvm/lib/ProfileData/InstrProf.cpp

1148

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.

[MaskRay]

Can you give concrete instructions to reproduce the linker error?

[YolandaCY]

In D107034#2945640, @MaskRay wrote:

Can you give concrete instructions to reproduce the linker error?

I tried to create a small reproducer but still encountered several issues. The linker error may be explained with below example:

class Str {
  int size;

public:
  char *p;

  Str(int size) {
    p = new char[size];
  }

  ~Str() {
    delete[] p;
  }

  void assign_mem(char *a, char *b, int size) {
    memcpy(a, b, size);
  }

  int assign(Str &b) {
    if (b.size < size) return -1;
    assign_mem(p, b.p, size);
    return 1;
  }
};

If function assign inlines assign_mem, it does contain mem op and should require a value profile var, but not vice versa.
The case encounterd the linker error is that a function similar to assign (e.g. basic_string::operator=) does not inline the callee assign_mem in the splitted regular LTO link, thus it will generate a __profd_assign* with private linkage, while in a later ThinLTO module, the assign inlined the callee containing mem op, thus it may generate a __profd_assign with non-private linkage (due to NS!=0). During the final link together with both regular and thin LTO modules for COFF, symbols from regular LTO are inserted earlier as prevailing, and the private __profd_assign* is not inserted. 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.

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.

I'm implementing a CL to build Chromium that depends on this. The steps described in the CL should help reproduce the error if you are interested @MaskRay. The original error looks like following:

lld-link: error: undefined symbol: __profd_??4?$basic_string@DU?$char_traits@D@__1@std@@V?$allocator@D@23@@__1@std@@QEAAAEAV012@AEBV012@@Z
>>> referenced by .\..\..\buildtools\third_party\libc++\trunk\include\string:2401
>>>               ./chrome.dll.lto.147.obj:(public: class std::__1::basic_string<char, struct std::__1::char_traits<char>, class std::__1::allocator<char>> & __cdecl std::td::__1::allocator<char>>::operator=(class std::__1::basic_string<char, struct std::__1::char_traits<char>, class std::__1::allocator<char>> const &))
>>> referenced by .\..\..\buildtools\third_party\libc++\trunk\include\string:2401
>>>               ./chrome.dll.lto.147.obj:(public: class std::__1::basic_string<char, struct std::__1::char_traits<char>, class std::__1::allocator<char>> & __cdecl std::td::__1::allocator<char>>::operator=(class std::__1::basic_string<char, struct std::__1::char_traits<char>, class std::__1::allocator<char>> const &))

Please let me know if more information is needed.

[MaskRay]

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.

Does your change make regular.o __profc_assign linkonce_odr?
Which variable in a non-prevailing comdat is referenced, causing an undefined symbol error?

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?

[MaskRay]

llvm/lib/ProfileData/InstrProf.cpp

1149

According to your description, various profc could cause linker errors. So the comment "The IRPGO flag var (__llvm_profile_raw_version) has external linkage," is incomplete/incorrect.

[MaskRay]

I edited my comment a bit so your email notification may not catch it.
This looks to me a PE/COFF specific issue. I wonder whether we can drop link.exe support for PGO / CSPGO.
link.exe has the limitation "non-leader members of a comdat must be private".
I recall that PE/COFF can entirely use the ELF scheme if we can lift the limitation.
(https://lists.llvm.org/pipermail/llvm-dev/2021-May/150758.html)

[YolandaCY]

In 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. I can have a try later.
The getVarName for profile variables will add FunHash as suffix in this change.

[YolandaCY]

In D107034#2953729, @MaskRay wrote:

I edited my comment a bit so your email notification may not catch it.
This looks to me a PE/COFF specific issue. I wonder whether we can drop link.exe support for PGO / CSPGO.
link.exe has the limitation "non-leader members of a comdat must be private".
I recall that PE/COFF can entirely use the ELF scheme if we can lift the limitation.
(https://lists.llvm.org/pipermail/llvm-dev/2021-May/150758.html)

I think I used lld-link.exe, not MSVC's link.exe. As mentioned in the link,

lld-link doesn't have this limitation.

I hope my last reply will help resolve some confusion.

[MaskRay]

In D107034#2954190, @YolandaCY wrote:

In 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, perhaps due to explicit instantiation of a template (GVA_StrongODR).
Since comdat renaming is disabled in this case, profd should not be private.

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. I can have a try later.
The getVarName for profile variables will add FunHash as suffix in this change.

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

linkonce_odr functions are generally renamed.
The linkage in question is weak_odr, so the renaming is disabled for canRenameComdatFunc.

So now this change looks strange to me:

if (!DoHashBasedCounterSplit || !enablesValueProfiling(*M) ||
    !canRenameComdatFunc(*F))
  return (Prefix + Name).str();

canRenameComdatFunc(*F) is false.

[MaskRay]

Perhaps the right fix is D108432. I need to update its description.

[YolandaCY]

In D107034#2956440, @MaskRay wrote:

Perhaps the right fix is D108432. I need to update its description.

Thank you for the patch. I will have a try with this fix and get back to you.

There might be multiple solutions to resolve the issues. Regardingless the linker error, the issue with IRPGO flag missing should be addressed I think. Does that sound good to you from CSPGO point of view?

[MaskRay]

About this patch M.addModuleFlag(llvm::Module::Warning, "EnableValueProfiling", 1);: I am not so sure we should do it.

Note that we only set "EnableValueProfiling" to 0 for the clang -fprofile-instr-generate case. The value is not set to 1 (without poking into the internal -enable-value-profiling option).

clang -fprofile-generate with value profiling doesn't set this module flag, so -fcs-profile-generate doesn't need to set it, either.

lld/COFF/LTO.cpp

226

drop unrelated change

[YolandaCY]

In D107034#2956730, @MaskRay wrote:

About this patch M.addModuleFlag(llvm::Module::Warning, "EnableValueProfiling", 1);: I am not so sure we should do it.

From enablesValueProfiling both IRPGOFlag and EnableValueProfiling mudule flag are examined as indicators. My understanding IRPGOFlag should imply "EnableValueProfiling" to 1.

This alternative solution was recommended by Reid, and no objection from Xu. Could you help clarify with Fangrui? @rnk @xur

Note that we only set "EnableValueProfiling" to 0 for the clang -fprofile-instr-generate case. The value is not set to 1 (without poking into the internal -enable-value-profiling option).

clang -fprofile-generate with value profiling doesn't set this module flag, so -fcs-profile-generate doesn't need to set it, either.

I think I have clarified why this change is unnecessary to PGO in previous reply.

I can drop the unrelated change regarding /lldsavetemps for sure. That should be an easy fix.

[MaskRay]

In D107034#2956812, @YolandaCY wrote:

In D107034#2956730, @MaskRay wrote:

About this patch M.addModuleFlag(llvm::Module::Warning, "EnableValueProfiling", 1);: I am not so sure we should do it.

From enablesValueProfiling both IRPGOFlag and EnableValueProfiling mudule flag are examined as indicators. My understanding IRPGOFlag should imply "EnableValueProfiling" to 1.

This alternative solution was recommended by Reid, and no objection from Xu. Could you help clarify with Fangrui? @rnk @xur

"EnableValueProfiling" is set to 0 by -fprofile-instr-generate to enable a size saving optimization (D102818).

IR profiling doesn't set 1. It is inconsistent for CSPGO to set "EnableValueProfiling" to 1, while -fprofile-generate keeps omitting the module flag.
-disable-vp can disable value profiling. This patch doesn't respect that option.

In addition, I think changing the condition in getVarName is incorrect. Coverage and PGO have different use cases while share some infrastructure (InstrProfiling.cpp).
Coverage suppresses renaming to have better coverage results. The decision is independent of whether value profiling is enabled or not.
On the other hand, PGO's preferring renaming behavior is also independent of value profiling.

Some confusion here is that -fprofile-instr-generate was used to do PGO (perhaps all groups except Apple have migrated away).
When "EnableValueProfiling" was introduced, I think the intention was to prevent breakage to them.
Otherwise, PGO use cases could just ignore "EnableValueProfiling" everywhere.

Actually, I am thinking of dropping "EnableValueProfiling". It needs some COFF side work (D44543) (or we just unsupport that use case).

Note that we only set "EnableValueProfiling" to 0 for the clang -fprofile-instr-generate case. The value is not set to 1 (without poking into the internal -enable-value-profiling option).

clang -fprofile-generate with value profiling doesn't set this module flag, so -fcs-profile-generate doesn't need to set it, either.

I think I have clarified why this change is unnecessary to PGO in previous reply.

I can drop the unrelated change regarding /lldsavetemps for sure. That should be an easy fix.

As previously explained, I think the patch is not correct. So keeping the "Request Changes" state.

[xur]

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?

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

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

[YolandaCY]

In 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, if we have concern on using EnableValueProfiling flag.
Not sure if this looks better?

[MaskRay]

In D107034#2958599, @YolandaCY wrote:

In 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, if we have concern on using EnableValueProfiling flag.
Not sure if this looks better?

I am afraid https://reviews.llvm.org/D107034?vs=on&id=363777#toc is incorrect, either.

My understanding below. @xur is the expert and may correct me:)

ThinLTO+CSPGO instrumentation is:

# After regular PGO instrumentation, profile collection, llvm-profdata merge
...
clang -flto=thin -fprofile-use=... -fcs-profile-generate=... a.c b.c # ThinLTO prelink phase
clang -flto=thin -fcs-profile-generate=... a.o b.o # ThinLTO postlink (backend compile) phase

The ThinLTO bitcode files need the -fcs-profile-generate= option to set __llvm_profile_raw_version in the PGOInstrumentationGenCreateVar pass.

ld.lld does comdat selection but the prevailing/non-prevailing decisions do not drop non-prevailing __llvm_profile_raw_version variables from bitcode files.

During ThinLTO postlink (backend compile), InstrProfiling.cpp doesn't need to add __llvm_profile_raw_version, because the variable is already there.

---

I think we need better documentation for CSPGO.

[YolandaCY]

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

[YolandaCY]

change flag name and remove unrelated fix

[xur]

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

[xur]

I sent my version of patch as https://reviews.llvm.org/D108581

[YolandaCY]

Issue is fixed at D108581.

lld/COFF/LTO.cpp

for (unsigned i = 0; i != maxTasks; ++i) {
     if (files[i])
       objBuf = files[i]->getBuffer();
     else
       objBuf = buf[i];
     if (objBuf.empty())
       continue;
 
     if (config->saveTemps)
-      saveBuffer(buf[i], ltoObjName);
+      saveBuffer(objBuf, ltoObjName);
     ret.push_back(make<ObjFile>(MemoryBufferRef(objBuf, ltoObjName)));
   }
 
   return ret;
 }

llvm/lib/ProfileData/InstrProf.cpp

 
 // Create a COMDAT variable INSTR_PROF_RAW_VERSION_VAR to make the runtime
 // aware this is an ir_level profile so it can set the version flag.
 void createIRLevelProfileFlagVar(Module &M, bool IsCS,
                                  bool InstrEntryBBEnabled) {
   const StringRef VarName(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
   Type *IntTy64 = Type::getInt64Ty(M.getContext());
   uint64_t ProfileVersion = (INSTR_PROF_RAW_VERSION | VARIANT_MASK_IR_PROF);
-  if (IsCS)
+  if (IsCS) {
     ProfileVersion |= VARIANT_MASK_CSIR_PROF;
+
+    // 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
+    // regular and thin LTO modules) compilation. This symbol may be accessed
+    // in PGO Instrumentation phase by isIRPGOFlagSet. Simply keep it will cause
+    // a duplicate symbol error on ELF. Try set the ValueProfiling module flag
+    // that can be evaluated by enablesValueProfiling instead.
+    M.addModuleFlag(llvm::Module::Warning, "EnableValueProfiling", 1);
+  }
   if (InstrEntryBBEnabled)
     ProfileVersion |= VARIANT_MASK_INSTR_ENTRY;
   auto IRLevelVersionVariable = new GlobalVariable(
       M, IntTy64, true, GlobalValue::WeakAnyLinkage,
       Constant::getIntegerValue(IntTy64, APInt(64, ProfileVersion)), VarName);
   IRLevelVersionVariable->setVisibility(GlobalValue::DefaultVisibility);
   Triple TT(M.getTargetTriple());
   if (TT.supportsCOMDAT()) {

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

for (unsigned I = 0, E = Names->getNumOperands(); I < E; ++I) {
 
     Name->setLinkage(GlobalValue::PrivateLinkage);
     ReferencedNames.push_back(Name);
     NC->dropAllReferences();
   }
   CoverageNamesVar->eraseFromParent();
 }
 
+static uint64_t getIntModuleFlagOrZero(const Module &M, StringRef Flag) {
+  auto *MD = dyn_cast_or_null<ConstantAsMetadata>(M.getModuleFlag(Flag));
+  if (!MD)
+    return 0;
+
+  // If the flag is a ConstantAsMetadata, it should be an integer representable
+  // in 64-bits.
+  return cast<ConstantInt>(MD->getValue())->getZExtValue();
+}
+
+static bool enablesValueProfiling(const Module &M) {
+  return getIntModuleFlagOrZero(M, "EnableValueProfiling") != 0 ||
+         isIRPGOFlagSet(&M);
+}
+
 /// Get the name of a profiling variable for a particular function.
 static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
   StringRef NamePrefix = getInstrProfNameVarPrefix();
   StringRef Name = Inc->getName()->getName().substr(NamePrefix.size());
   Function *F = Inc->getParent()->getParent();
   Module *M = F->getParent();
-  if (!DoHashBasedCounterSplit || !isIRPGOFlagSet(M) ||
+  if (!DoHashBasedCounterSplit || !enablesValueProfiling(*M) ||
       !canRenameComdatFunc(*F))
     return (Prefix + Name).str();
   uint64_t FuncHash = Inc->getHash()->getZExtValue();
   SmallVector<char, 24> HashPostfix;
   if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix)))
     return (Prefix + Name).str();
   return (Prefix + Name + "." + Twine(FuncHash)).str();
 }
 
-static uint64_t getIntModuleFlagOrZero(const Module &M, StringRef Flag) {
-  auto *MD = dyn_cast_or_null<ConstantAsMetadata>(M.getModuleFlag(Flag));
-  if (!MD)
-    return 0;
-
-  // If the flag is a ConstantAsMetadata, it should be an integer representable
-  // in 64-bits.
-  return cast<ConstantInt>(MD->getValue())->getZExtValue();
-}
-
-static bool enablesValueProfiling(const Module &M) {
-  return isIRPGOFlagSet(&M) ||
-         getIntModuleFlagOrZero(M, "EnableValueProfiling") != 0;
-}
 
 // Conservatively returns true if data variables may be referenced by code.
 static bool profDataReferencedByCode(const Module &M) {
   return enablesValueProfiling(M);
 }
 
 static inline bool shouldRecordFunctionAddr(Function *F) {
   // Only record function addresses if IR PGO is enabled or if clang value

llvm/test/Instrumentation/InstrProfiling/comdat.ll

 ; There are two main cases where comdats are necessary:
 ; 1. standard inline functions (weak_odr / linkonce_odr)
 ; 2. available externally functions (C99 inline / extern template / dllimport)
 ; Check that we do the right thing for the two object formats with comdats, ELF
 ; and COFF.
 ;
 ; Test comdat functions. Non-comdat functions are tested in linkage.ll.
 ; RUN: split-file %s %t
 ; RUN: cat %t/main.ll %t/disable.ll > %t0.ll
 ; RUN: cat %t/main.ll %t/enable.ll > %t1.ll
 ; RUN: opt < %t0.ll -mtriple=x86_64-linux -passes=instrprof -S | FileCheck %s --check-prefixes=ELF
 ; RUN: opt < %t1.ll -mtriple=x86_64-linux -passes=instrprof -S | FileCheck %s --check-prefixes=ELF
-; RUN: opt < %t0.ll -mtriple=x86_64-windows -passes=instrprof -S | FileCheck %s --check-prefixes=COFF
-; RUN: opt < %t1.ll -mtriple=x86_64-windows -passes=instrprof -S | FileCheck %s --check-prefixes=COFF
+; RUN: opt < %t0.ll -mtriple=x86_64-windows -passes=instrprof -S | FileCheck %s --check-prefixes=COFF0
+; RUN: opt < %t1.ll -mtriple=x86_64-windows -passes=instrprof -S | FileCheck %s --check-prefixes=COFF1
 
 ;--- main.ll
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
 
 $foo_inline = comdat any
 $foo_extern = comdat any
 
 @__profn_foo_inline = linkonce_odr hidden constant [10 x i8] c"foo_inline"

 ; COFF0: @__profd_foo_inline = private global {{.*}}, section ".lprfd$M", comdat($__profc_foo_inline), align 8
 ; COFF1: @__profc_foo_inline = linkonce_odr hidden global {{.*}}, section ".lprfc$M", comdat, align 8
 ; COFF1: @__profd_foo_inline = linkonce_odr hidden global {{.*}}, section ".lprfd$M", comdat, align 8
 define weak_odr void @foo_inline() comdat {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
-; ELF:   @__profc_foo_extern = linkonce_odr hidden global{{.*}}, section "__llvm_prf_cnts", comdat, align 8
+; ELF0:   @__profc_foo_extern = linkonce_odr hidden global{{.*}}, section "__llvm_prf_cnts", comdat, align 8
+; ELF1:   @__profc_foo_extern.0 = linkonce_odr hidden global{{.*}}, section "__llvm_prf_cnts", comdat, align 8
 ; ELF0:  @__profd_foo_extern = private global{{.*}}, section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
-; ELF1:  @__profd_foo_extern = linkonce_odr hidden global{{.*}}, section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
-; COFF:  @__profc_foo_extern = linkonce_odr hidden global{{.*}}, section ".lprfc$M", comdat, align 8
+; ELF1:  @__profd_foo_extern.0 = linkonce_odr hidden global{{.*}}, section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
+; COFF0:  @__profc_foo_extern = linkonce_odr hidden global{{.*}}, section ".lprfc$M", comdat, align 8
+; COFF1:  @__profc_foo_extern.0 = linkonce_odr hidden global{{.*}}, section ".lprfc$M", comdat, align 8
 ; COFF0: @__profd_foo_extern = private global{{.*}}, section ".lprfd$M", comdat($__profc_foo_extern), align 8
-; COFF1: @__profd_foo_extern = linkonce_odr hidden global{{.*}}, section ".lprfd$M", comdat, align 8
+; COFF1: @__profd_foo_extern.0 = linkonce_odr hidden global{{.*}}, section ".lprfd$M", comdat, align 8
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; ELF:   @llvm.compiler.used = appending global {{.*}} @__profd_foo_inline {{.*}} @__profd_foo_extern
 ; COFF0: @llvm.compiler.used = appending global {{.*}} @__llvm_profile_runtime_user {{.*}} @__profd_foo_inline {{.*}} @__profd_foo_extern
 ; COFF1: @llvm.used = appending global {{.*}} @__llvm_profile_runtime_user {{.*}} @__profd_foo_inline {{.*}} @__profd_foo_extern
 
 ;--- disable.ll
 !llvm.module.flags = !{!0}
 !0 = !{i32 2, !"EnableValueProfiling", i32 0}
 
 ;--- enable.ll
 !llvm.module.flags = !{!0}
 !0 = !{i32 2, !"EnableValueProfiling", i32 1}

llvm/test/Transforms/PGOProfile/lto_cspgo_gen.ll

+; REQUIRES: x86-registered-target
+
+; RUN: opt -passes='thinlto-pre-link<O2>' --cs-profilegen-file=alloc -cspgo-kind=cspgo-instr-gen-pipeline -module-summary %s -o %t.bc
+; RUN: llvm-dis %t.bc -o - | FileCheck %s --check-prefix=VALUEPROFILE
+
+; RUN: llvm-lto2 run -lto-cspgo-profile-file=alloc -lto-cspgo-gen -save-temps -o %t %t.bc \
+; RUN:   -r=%t.bc,f,pl \
+; RUN:   -r=%t.bc,__llvm_profile_filename,x \
+; RUN:   -r=%t.bc,__llvm_profile_raw_version,x
+; RUN: llvm-dis %t.0.0.preopt.bc -o - | FileCheck %s --check-prefix=VALUEPROFILE
+
+; VALUEPROFILE: {{.*}} !"EnableValueProfiling", i32 1}
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nofree norecurse nosync nounwind readnone uwtable willreturn mustprogress
+define dso_local i32 @f() local_unnamed_addr #0 {
+entry:
+  ret i32 1
+}
+
+attributes #0 = { "target-cpu"="x86-64" }
+
+!llvm.module.flags = !{!0, !1, !2}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
+!1 = !{i32 7, !"uwtable", i32 1}
+!2 = !{i32 1, !"ThinLTO", i32 0}