Page MenuHomePhabricator

SampleFDO] place the discriminator flag variable into the used list.
ClosedPublic

Authored by xur on Jun 9 2021, 1:02 PM.

Details

Summary

We create flag variable "llvm_fs_discriminator" in the binary
to indicate that FSAFDO hierarchical discriminators are used.

This variable might be GC'ed by the linker since it is not explicitly
reference. I initially added the var to the use list in pass
MIRFSDiscriminator but it did not work. It turned out the used global
list is collected in lowering (before MIR pass) and then emitted in
the end of pass pipeline.

In this patch, we use a "common" linkage for this variable so that
it will be GC'ed by the linker.

Diff Detail

Event Timeline

xur created this revision.Jun 9 2021, 1:02 PM
xur requested review of this revision.Jun 9 2021, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2021, 1:02 PM
hoy added inline comments.Jun 10 2021, 10:29 AM
llvm/lib/Transforms/Utils/SampleProfileLoaderBaseUtil.cpp
171 ↗(On Diff #350983)

There's a CommonLinkage type and from its description, it looks like command variables should not be GC-ed but they can only have a zero value. Not sure if -Wl,--export-dynamic-symbol is needed with the common linkage type.

common
“common” linkage is most similar to “weak” linkage, but they are used for tentative definitions in C, such as “int X;” at global scope. Symbols with “common” linkage are merged in the same way as weak symbols, and they may not be deleted if unreferenced. common symbols may not have an explicit section, must have a zero initializer, and may not be marked ‘constant’. Functions and aliases may not have common linkage.

xur added inline comments.Jun 11 2021, 1:28 PM
llvm/lib/Transforms/Utils/SampleProfileLoaderBaseUtil.cpp
171 ↗(On Diff #350983)

Hongtao, thanks for pointing this out. I just tried and seemed "common" linkage worked -- I don't need to put the variable into used list and we don't need --export-dynamic-symbol to suppress the GC.

Using common linkage is a better solution here. I'll update the patch shortly.

xur updated this revision to Diff 351549.Jun 11 2021, 1:33 PM
xur edited the summary of this revision. (Show Details)
hoy accepted this revision.Jun 11 2021, 2:27 PM
hoy added inline comments.
llvm/lib/Transforms/Utils/SampleProfileLoaderBaseUtil.cpp
171 ↗(On Diff #350983)

LGTM, thanks for trying this out.

This revision is now accepted and ready to land.Jun 11 2021, 2:27 PM
wenlei added inline comments.Jun 11 2021, 9:11 PM
llvm/lib/Transforms/Utils/SampleProfileLoaderBaseUtil.cpp
171 ↗(On Diff #350983)

Looks like similar things like createIRLevelProfileFlagVar and createProfileFileNameVar are using ExternalLinkage which would also suppress GC. (there's a check against supportsCOMDAT too). Is there a convention for these?

This revision was landed with ongoing or failed builds.Jun 15 2021, 2:55 PM
This revision was automatically updated to reflect the committed changes.

ELF linkers can GC common symbols. Common symbols are more and more irrelevant nowadays since GCC and Clang have switched to -fno-common.

I don't know FSAFDO. Can you show the clang + ld command line so that I can play with? :)

xur added a comment.Jun 15 2021, 4:25 PM

@Fangrui: My initial version was to place it into the use list. But Hoy
suggested that I use CommonLinage.
From llvm reference manual, it seems that linker should GC a common symbol.
Can you read the message and add comments there?
https://reviews.llvm.org/D103988

CommonLinkage doesn't work.

Whether appendToUsed is needed depends on whether the runtime has a live section referencing __llvm_fs_discriminator__.
If the runtime has the guarantee, no need for appendToUsed.

Otherwise, the first revision appendToUsed with WeakODRLinkage looks good to me.

appendToUsed with ExternalLinkage in a comdat works as well, but comdat will need extra code when Mach-O wants this feature.

MaskRay added inline comments.Jun 15 2021, 4:52 PM
llvm/lib/Transforms/Utils/SampleProfileLoaderBaseUtil.cpp
171 ↗(On Diff #350983)

"and they may not be deleted if unreferenced." This is the compiler behavior, not the linker behavior.

CommonLinkage is an externally visible definition not necessarily has duplicates (linkonce/linkonce_odr/available_externally) so the compiler cannot remove it. In the compiler, a CommonLinkage variable is considered interposable (so various IPO is suppressed). The linker can GC it.

xur added a comment.Jun 15 2021, 4:58 PM

CommonLinkage doesn't work.

Whether appendToUsed is needed depends on whether the runtime has a live section referencing __llvm_fs_discriminator__.
If the runtime has the guarantee, no need for appendToUsed.

Otherwise, the first revision appendToUsed with WeakODRLinkage looks good to me.

appendToUsed with ExternalLinkage in a comdat works as well, but comdat will need extra code when Mach-O wants this feature.

Fangri, thanks for reviewing this. This variable will NOT be referenced by compiler runtime library -- only be referenced by AutoFDO offline tool (to generate AFDO profile).

It seems that Fangri (as a linker expert) prefers my first implementation.

I'm OK either way. Hoy: what is your thought?

hoy added inline comments.Jun 15 2021, 4:59 PM
llvm/lib/Transforms/Utils/SampleProfileLoaderBaseUtil.cpp
171 ↗(On Diff #350983)

"and they may not be deleted if unreferenced." This is the compiler behavior, not the linker behavior.

You mean that's the LTO behavior? Compiler would only remove unreferenced static variables when not seeing a global picture.

Good to know the linker doesn't honor that. Thanks for pointing that out.

hoy added a comment.Jun 15 2021, 5:01 PM

CommonLinkage doesn't work.

Whether appendToUsed is needed depends on whether the runtime has a live section referencing __llvm_fs_discriminator__.
If the runtime has the guarantee, no need for appendToUsed.

Otherwise, the first revision appendToUsed with WeakODRLinkage looks good to me.

appendToUsed with ExternalLinkage in a comdat works as well, but comdat will need extra code when Mach-O wants this feature.

Fangri, thanks for reviewing this. This variable will NOT be referenced by compiler runtime library -- only be referenced by AutoFDO offline tool (to generate AFDO profile).

It seems that Fangri (as a linker expert) prefers my first implementation.

I'm OK either way. Hoy: what is your thought?

Sounds good to me. Didn't know that linker could GC common symbols. Sorry about that.