[InstrProfiling] Put instrumentation into comdat group with function

[phosek]

Summary

Currently, per-function instrumentation data don't use section groups
except for functions that already are in one and symbols with external
linkage. Consequentially, profiling instrumentation doesn't support
--gc-sections: if the function symbol is discarded, linker doesn't know
what other (instrumentation) data needs to be discarded as well. This
breaks any code that relies on --gc-sections semantics, and even for
other code that doesn't can lead to size increase.

This change moves per-function instrumentation data into its own section
group together with the function itself. This allows all instrumentation
to "travel together" with the symbol. If linker decides to discard the
symbol, it'll discard the entire group including all sections with the
instrumentation. The same strategy is already used by sancov.

[davidxl]

llvm/lib/ProfileData/InstrProf.cpp

1054

See the comments in the code deleted by this patch.

Here you reverse the logic. IIRC, it will greatly increase the size of the profile data as the counter and per function data for externally available symbols won't be commoned.

[phosek]

llvm/lib/ProfileData/InstrProf.cpp

1054

I've reverted that part of the change and checked on an example that those symbols are now being commoned.

[davidxl]

Do you have a functional test case to show what this patch is trying to accomplish/fix?

[phosek]

In D64045#1569136, @davidxl wrote:

Do you have a functional test case to show what this patch is trying to accomplish/fix?

What kind of test do you have in mind? You mean and E2E test in LLVM or a real-world use case? The only issue with E2E test is that it's going to require linker to verify that we can indeed GC the unused symbols together with the relevant part of the instrumentation, is that OK?

[davidxl]

Some end-to-end tests as in compiler-rt are probably helpful here.

[pcc]

Should this be using !associated metadata on ELF instead of comdats?

[phosek]

In D64045#1766212, @pcc wrote:

Should this be using !associated metadata on ELF instead of comdats?

I agree that !associated metadata is a better fit than comdats, the problem is that !associated metadata is lowered to SHF_LINK_ORDER which is not the correct representation; the intended use case for SHF_LINK_ORDER is to control the section output order (see https://groups.google.com/g/generic-abi/c/avQCrdIALKE) which is not the case here. I think the most appropriate representation in ELF is section group (just a plain section group, not comdat), but there's no way to currently express that in LLVM IR. One option would be to change the lowering of !associated metadata to use section groups and then use !associated metadata in this change.

[phosek]

Superseded by D76802.

llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h

InstrProfiling(const InstrProfOptions &Options, bool IsCS = false)
       : Options(Options), IsCS(IsCS) {}
 
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
   bool run(Module &M, const TargetLibraryInfo &TLI);
 
 private:
   InstrProfOptions Options;
   Module *M;
+  std::string ModuleUniqueId;
   Triple TT;
   const TargetLibraryInfo *TLI;
   struct PerFunctionProfileData {
     uint32_t NumValueSites[IPVK_Last + 1];
     GlobalVariable *RegionCounters = nullptr;
     GlobalVariable *DataVar = nullptr;
 
     PerFunctionProfileData() {

llvm/lib/ProfileData/InstrProf.cpp

 
 bool needsComdatForCounter(const Function &F, const Module &M) {
   if (F.hasComdat())
     return true;
 
   if (!Triple(M.getTargetTriple()).supportsCOMDAT())
     return false;
 
-  // See createPGOFuncNameVar for more details. To avoid link errors, profile
-  // counters for function with available_externally linkage needs to be changed
-  // to linkonce linkage. On ELF based systems, this leads to weak symbols to be
-  // created. Without using comdat, duplicate entries won't be removed by the
-  // linker leading to increased data segement size and raw profile size. Even
-  // worse, since the referenced counter from profile per-function data object
-  // will be resolved to the common strong definition, the profile counts for
-  // available_externally functions will end up being duplicated in raw profile
-  // data. This can result in distorted profile as the counts of those dups
-  // will be accumulated by the profile merger.
-  GlobalValue::LinkageTypes Linkage = F.getLinkage();
-  if (Linkage != GlobalValue::ExternalWeakLinkage &&
-      Linkage != GlobalValue::AvailableExternallyLinkage)
-    return false;
-
   return true;
 }
 
 // Check if INSTR_PROF_RAW_VERSION_VAR is defined.
 bool isIRPGOFlagSet(const Module *M) {
   auto IRInstrVar =
       M->getNamedGlobal(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
   if (!IRInstrVar || IRInstrVar->isDeclaration() ||
       IRInstrVar->hasLocalLinkage())
     return false;
 

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

if (auto *F = M.getFunction(
           Intrinsic::getName(llvm::Intrinsic::instrprof_value_profile)))
     if (!F->use_empty())
       return true;
   return false;
 }
 
 bool InstrProfiling::run(Module &M, const TargetLibraryInfo &TLI) {
   this->M = &M;
+  ModuleUniqueId = getUniqueModuleId(this->M);
   this->TLI = &TLI;
   NamesVar = nullptr;
   NamesSize = 0;
   ProfileDataMap.clear();
   UsedVars.clear();
   getMemOPSizeRangeFromOption(MemOPSizeRange, MemOPSizeRangeStart,
                               MemOPSizeRangeLast);
   TT = Triple(M.getTargetTriple());
InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   // in mind that this pass may run before the inliner, so we need to create a
   // new comdat group for the counters and profiling data. If we use the comdat
   // of the parent function, that will result in relocations against discarded
   // sections.
   Comdat *Cmdt = nullptr;
   GlobalValue::LinkageTypes CounterLinkage = Linkage;
   if (needsComdatForCounter(*Fn, *M)) {
     StringRef CmdtPrefix = getInstrProfComdatPrefix();
+    GlobalValue::LinkageTypes Linkage = Fn->getLinkage();
+    if (Linkage == GlobalValue::ExternalWeakLinkage ||
+        Linkage == GlobalValue::AvailableExternallyLinkage) {
       if (TT.isOSBinFormatCOFF()) {
         // For COFF, the comdat group name must be the name of a symbol in the
         // group. Use the counter variable name, and upgrade its linkage to
         // something externally visible, like linkonce_odr.
         CmdtPrefix = getInstrProfCountersVarPrefix();
         CounterLinkage = GlobalValue::LinkOnceODRLinkage;
       }
       Cmdt = M->getOrInsertComdat(getVarName(Inc, CmdtPrefix));
+    } else {
+      Cmdt = GetOrCreateFunctionComdat(*Fn, TT, ModuleUniqueId);
+    }
   }
 
   uint64_t NumCounters = Inc->getNumCounters()->getZExtValue();
   LLVMContext &Ctx = M->getContext();
   ArrayType *CounterTy = ArrayType::get(Type::getInt64Ty(Ctx), NumCounters);
 
   // Create the counters variable.
   auto *CounterPtr =

llvm/test/Instrumentation/InstrProfiling/PR23499.ll

 ;; Check that PGO instrumented variables associated with linkonce odr
 ;; functions are placed in the same comdat section.
 
 
 ; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -instrprof -S | FileCheck %s
 ; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -passes=instrprof -S | FileCheck %s
 ; RUN: opt < %s -mtriple=x86_64-unknown-linux -instrprof -S | FileCheck %s
 ; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=instrprof -S | FileCheck %s
 ; RUN: opt < %s -mtriple=x86_64-pc-win32-coff -instrprof -S | FileCheck %s --check-prefix=COFF
 ; RUN: opt < %s -mtriple=x86_64-pc-win32-coff -passes=instrprof -S | FileCheck %s --check-prefix=COFF
 
 $_Z3barIvEvv = comdat any
 
 @__profn__Z3barIvEvv = linkonce_odr hidden constant [11 x i8] c"_Z3barIvEvv", align 1
 
 ; CHECK-NOT: __profn__Z3barIvEvv
-; CHECK: @__profc__Z3barIvEvv = linkonce_odr hidden global [1 x i64] zeroinitializer, section "{{.*}}__llvm_prf_cnts", comdat($__profv__Z3barIvEvv), align 8
-; CHECK: @__profd__Z3barIvEvv = linkonce_odr hidden global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}__llvm_prf_data{{.*}}", comdat($__profv__Z3barIvEvv), align 8
+; CHECK: @__profc__Z3barIvEvv = linkonce_odr hidden global [1 x i64] zeroinitializer, section "{{.*}}__llvm_prf_cnts", comdat($_Z3barIvEvv), align 8
+; CHECK: @__profd__Z3barIvEvv = linkonce_odr hidden global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}__llvm_prf_data{{.*}}", comdat($_Z3barIvEvv), align 8
 ; CHECK: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}__llvm_prf_names"
 
 
 ; COFF-NOT: __profn__Z3barIvEvv
-; COFF: @__profc__Z3barIvEvv = linkonce_odr dso_local global [1 x i64] zeroinitializer, section "{{.*}}prfc$M", comdat, align 8
-; COFF: @__profd__Z3barIvEvv = internal global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}prfd{{.*}}", comdat($__profc__Z3barIvEvv), align 8
+; COFF: @__profc__Z3barIvEvv = internal global [1 x i64] zeroinitializer, section "{{.*}}prfc$M", comdat($_Z3barIvEvv), align 8
+; COFF: @__profd__Z3barIvEvv = internal global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 4947693190065689389, i64 0, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__Z3barIvEvv, i32 0, i32 0), i8*{{.*}}, i8* null, i32 1, [2 x i16] zeroinitializer }, section "{{.*}}prfd{{.*}}", comdat($_Z3barIvEvv), align 8
 
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32) #1
 
 define linkonce_odr void @_Z3barIvEvv() comdat {
 entry:
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([11 x i8], [11 x i8]* @__profn__Z3barIvEvv, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }

llvm/test/Instrumentation/InstrProfiling/comdat.ll

 ; and COFF.
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
 
 $foo_inline = comdat any
 
 @__profn_foo_inline = linkonce_odr hidden constant [10 x i8] c"foo_inline"
 
-; ELF: @__profc_foo_inline = linkonce_odr hidden global{{.*}}, section "__llvm_prf_cnts", comdat($__profv_foo_inline), align 8
-; ELF: @__profd_foo_inline = linkonce_odr hidden global{{.*}}, section "__llvm_prf_data", comdat($__profv_foo_inline), align 8
-; COFF: @__profc_foo_inline = linkonce_odr dso_local global{{.*}}, section ".lprfc$M", comdat, align 8
-; COFF: @__profd_foo_inline = internal global{{.*}}, section ".lprfd$M", comdat($__profc_foo_inline), align 8
+; ELF: @__profc_foo_inline = linkonce_odr hidden global{{.*}}, section "__llvm_prf_cnts", comdat($foo_inline), align 8
+; ELF: @__profd_foo_inline = linkonce_odr hidden global{{.*}}, section "__llvm_prf_data", comdat($foo_inline), align 8
+; COFF: @__profc_foo_inline = internal global{{.*}}, section ".lprfc$M", comdat($foo_inline), align 8
+; COFF: @__profd_foo_inline = internal global{{.*}}, section ".lprfd$M", comdat($foo_inline), 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
 }
 
 $foo_extern = comdat any
 
 @__profn_foo_extern = linkonce_odr hidden constant [10 x i8] c"foo_extern"

llvm/test/Instrumentation/InstrProfiling/linkage.ll

 @__profn_foo_weak = weak hidden constant [8 x i8] c"foo_weak"
 @"__profn_linkage.ll:foo_internal" = internal constant [23 x i8] c"linkage.ll:foo_internal"
 @__profn_foo_inline = linkonce_odr hidden constant [10 x i8] c"foo_inline"
 @__profn_foo_extern = linkonce_odr hidden constant [10 x i8] c"foo_extern"
 
 ; POSIX: @__profc_foo = hidden global
 ; POSIX: @__profd_foo = hidden global
 ; COFF: @__profc_foo = internal global
-; COFF-NOT: comdat
 ; COFF: @__profd_foo = internal global
 define void @foo() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; POSIX: @__profc_foo_weak = weak hidden global
 ; POSIX: @__profd_foo_weak = weak hidden global

 ; COFF: @"__profd_linkage.ll:foo_internal" = internal global
 define internal void @foo_internal() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([23 x i8], [23 x i8]* @"__profn_linkage.ll:foo_internal", i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 ; POSIX: @__profc_foo_inline = linkonce_odr hidden global
 ; POSIX: @__profd_foo_inline = linkonce_odr hidden global
-; COFF: @__profc_foo_inline = internal global{{.*}} section ".lprfc$M", align 8
-; COFF: @__profd_foo_inline = internal global{{.*}} section ".lprfd$M", align 8
+; COFF: @__profc_foo_inline = internal global{{.*}} section ".lprfc$M", comdat($foo_inline), align 8
+; COFF: @__profd_foo_inline = internal global{{.*}} section ".lprfd$M", comdat($foo_inline), align 8
 define linkonce_odr void @foo_inline() {
   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
 }
 
 ; LINUX: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_cnts", comdat($__profv_foo_extern), align 8
 ; LINUX: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_data", comdat($__profv_foo_extern), align 8
 ; MACHO: @__profc_foo_extern = linkonce_odr hidden global

llvm/test/Instrumentation/InstrProfiling/platform.ll

 ; RUN: opt < %s -mtriple=x86_64-pc-windows -passes=instrprof -S | FileCheck %s -check-prefix=WINDOWS
 
 @__profn_foo = hidden constant [3 x i8] c"foo"
 ; MACHO-NOT: __profn_foo
 ; ELF-NOT: __profn_foo
 ; WINDOWS-NOT: __profn_foo
 
 ; MACHO: @__profc_foo = hidden global [1 x i64] zeroinitializer, section "__DATA,__llvm_prf_cnts", align 8
-; ELF: @__profc_foo = hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", align 8
-; WINDOWS: @__profc_foo = internal global [1 x i64] zeroinitializer, section ".lprfc$M", align 8
+; ELF: @__profc_foo = hidden global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($foo), align 8
+; WINDOWS: @__profc_foo = internal global [1 x i64] zeroinitializer, section ".lprfc$M", comdat($foo), align 8
 
 ; MACHO: @__profd_foo = hidden {{.*}}, section "__DATA,__llvm_prf_data,regular,live_support", align 8
-; ELF: @__profd_foo = hidden {{.*}}, section "__llvm_prf_data", align 8
-; WINDOWS: @__profd_foo = internal global {{.*}}, section ".lprfd$M", align 8
+; ELF: @__profd_foo = hidden {{.*}}, section "__llvm_prf_data", comdat($foo), align 8
+; WINDOWS: @__profd_foo = internal global {{.*}}, section ".lprfd$M", comdat($foo), align 8
 
 ; ELF: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}__llvm_prf_names", align 1
 ; WINDOWS: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}lprfn$M", align 1
 
 define void @foo() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }

llvm/test/Transforms/PGOProfile/comdat_internal.ll

 ; RUN: opt < %s -pgo-instr-gen -instrprof -S | FileCheck %s
 ; RUN: opt < %s -passes=pgo-instr-gen,instrprof -S | FileCheck %s
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 $foo = comdat any
 ; CHECK: $foo = comdat any
 
 ; CHECK: $__llvm_profile_raw_version = comdat any
-; CHECK: $__profv__stdin__foo.[[FOO_HASH:[0-9]+]] = comdat any
 
 @bar = global i32 ()* @foo, align 8
 
 ; CHECK: @__llvm_profile_raw_version = constant i64 {{[0-9]+}}, comdat
 ; CHECK-NOT: __profn__stdin__foo
-; CHECK: @__profc__stdin__foo.[[FOO_HASH]] = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profv__stdin__foo.[[FOO_HASH]]), align 8
+; CHECK: @__profc__stdin__foo.[[FOO_HASH:[0-9]+]] = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($foo), align 8
 ; CHECK: @__profd__stdin__foo.[[FOO_HASH]] = private global { i64, i64, i64*, i8*, i8*, i32, [2 x i16] } { i64 -5640069336071256030, i64 [[FOO_HASH]], i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__stdin__foo.[[FOO_HASH]], i32 0, i32 0), i8* null
 ; CHECK-NOT: bitcast (i32 ()* @foo to i8*)
-; CHECK-SAME: , i8* null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profv__stdin__foo.[[FOO_HASH]]), align 8
+; CHECK-SAME: , i8* null, i32 1, [2 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($foo), align 8
 ; CHECK: @__llvm_prf_nm
 ; CHECK: @llvm.used
 
 define internal i32 @foo() comdat {
 entry:
   ret i32 1
 }