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

[xur]

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.

[hoy]

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]

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.

[hoy]

llvm/lib/Transforms/Utils/SampleProfileLoaderBaseUtil.cpp

171 ↗

(On Diff #350983)

LGTM, thanks for trying this out.

[wenlei]

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?

[MaskRay]

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]

@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

[MaskRay]

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]

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]

In D103988#2820735, @MaskRay wrote:

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]

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]

In D103988#2820746, @xur wrote:

In D103988#2820735, @MaskRay wrote:

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.

llvm/lib/CodeGen/MIRFSDiscriminator.cpp

bool MIRAddFSDiscriminators::runOnMachineFunction(MachineFunction &MF) {
   }
 
   if (Changed) {
     Module *M = MF.getFunction().getParent();
     const char *FSDiscriminatorVar = "__llvm_fs_discriminator__";
     if (!M->getGlobalVariable(FSDiscriminatorVar)) {
       auto &Context = M->getContext();
       // Create a global variable to flag that FSDiscriminators are used.
-      new GlobalVariable(*M, Type::getInt1Ty(Context), true,
-                         GlobalValue::WeakAnyLinkage,
-                         ConstantInt::getTrue(Context), FSDiscriminatorVar);
+      // Using "common" linkage so that it will not gc GC'ed.
+      new GlobalVariable(*M, Type::getInt1Ty(Context), false,
+                         GlobalValue::CommonLinkage,
+                         ConstantInt::getFalse(Context), FSDiscriminatorVar);
     }
 
     LLVM_DEBUG(dbgs() << "Num of FS Discriminators: " << NumNewD << "\n");
   }
 
   return Changed;
 }

llvm/test/CodeGen/X86/fsafdo_test1.ll

 ; RUN: llc -enable-fs-discriminator < %s | FileCheck %s
 ;
-; Check that fs-afdo discriminators are generated.
+;; Check that fs-afdo discriminators are generated.
 ; CHECK: .loc    1 7 3 is_stmt 0 discriminator 2 # foo.c:7:3
 ; Check: .loc    1 9 5 is_stmt 1 discriminator 2 # foo.c:9:5
 ; CHECK: .loc    1 9 5 is_stmt 0 discriminator 268435458 # foo.c:9:5
 ; CHECK: .loc    1 7 3 is_stmt 1 discriminator 3892314114 # foo.c:7:3
-; Check that variable __llvm_fs_discriminator__ is generated.
-; CHECK: .type   __llvm_fs_discriminator__,@object # @__llvm_fs_discriminator__
-; CHECK: .section        .rodata,"a",@progbits
-; CHECK: .weak   __llvm_fs_discriminator__
-; CHECK: __llvm_fs_discriminator__:
-; CHECK: .byte   1
-; CHECK: .size   __llvm_fs_discriminator__, 1
+;; Check that variable __llvm_fs_discriminator__ is generated.
+; CHECK: .type   __llvm_fs_discriminator__,@object
+; CHECK: .comm __llvm_fs_discriminator__,1,1
 
 target triple = "x86_64-unknown-linux-gnu"
 
 %struct.Node = type { %struct.Node* }
 
 define i32 @foo(%struct.Node* readonly %node, %struct.Node* readnone %root) !dbg !6 {
 entry:
   %cmp = icmp eq %struct.Node* %node, %root, !dbg !8

llvm/test/CodeGen/X86/fsafdo_test2.ll

 ;;
 ;; Check that fs-afdo discriminators are generated.
 ; CHECK: .loc    1 23 9 is_stmt 0 discriminator 1 # unroll.c:23:9
 ; CHECK: .loc    1 23 9 is_stmt 0 discriminator 3892314113 # unroll.c:23:9
 ; CHECK: .loc    1 23 9 is_stmt 0 discriminator 2818572289 # unroll.c:23:9
 ; CHECK: .loc    1 23 9 is_stmt 0 discriminator 3623878657 # unroll.c:23:9
 ;;
 ;; Check that variable __llvm_fs_discriminator__ is generated.
-; CHECK: .type   __llvm_fs_discriminator__,@object # @__llvm_fs_discriminator__
-; CHECK: .section        .rodata,"a",@progbits
-; CHECK: .weak   __llvm_fs_discriminator__
-; CHECK: __llvm_fs_discriminator__:
-; CHECK: .byte   1
-; CHECK: .size   __llvm_fs_discriminator__, 1
+; CHECK: .type   __llvm_fs_discriminator__,@object
+; CHECK: .comm __llvm_fs_discriminator__,1,1
 
 target triple = "x86_64-unknown-linux-gnu"
 
 @sum = dso_local local_unnamed_addr global i32 0, align 4
 
 declare i32 @bar(i32 %i) #0
 declare void @work(i32 %i) #2