[InstrProf] Use separate comdat group for data and counters

[rnk]

Summary

I hadn't realized that instrumentation runs before inlining, so we can't
use the function as the comdat group. Doing so can create relocations
against discarded sections when references to discarded __profc_
variables are inlined into functions outside the function's comdat
group.

In the future, perhaps we should consider standardizing the comdat group
names that ELF and COFF use. It will save object file size, since
__profv_$sym won't appear in the symbol table again.

[vsk]

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

Also, why is the selection kind here 'any' instead of 'exactmatch'? If the counter array sizes legitimately can vary (why?), then wouldn't we want 'largestsize'?

[xur]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

756

So with the patch, COFF and ELF have the same way of naming. Can we simplify the code by hosting the common code?

[rnk]

In D58737#1412734, @vsk wrote:

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

Also, why is the selection kind here 'any' instead of 'exactmatch'? If the counter array sizes legitimately can vary (why?), then wouldn't we want 'largestsize'?

I think if the data size varies, then we are out of luck. Every TU will produce the same data if the profile intrinsics are inserted at the same point in the pipeline. Also, both largestsize and exactmatch are COFF-only.

For code coverage, the instrprof.increment calls are inserted very early, and it is source-directed, so we should be pretty confident that all profile data for a given inline function is identical across the whole program. Of course, different versions of clang may insert counters differently, so if you link together object files produced by two versions of clang, you run the risk of having corrupt profile data.

For PGO, I have less confidence that that is true, but I believe InstrProf is carefully placed in the pipeline (before the main inliner pass, preinlining only) at such a point to reduce the likelihood that this happens. Of course, PGO already locks in the compiler version of the whole program. You can't optimize with instrumentation inserted by the last release of clang.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

756

It's not identical yet, though, but maybe it should be. For COFF, the comdat is __profc_$sym, but for ELF, it is __profv_$sym. However, they are very similar, and I can factor out the common code now without changing ELF to match COFF.

[rnk]

• share code

[rnk]

Oops, forgot to respond to this...

In D58737#1412734, @vsk wrote:

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

I agree, that wording seems very confusing. I probably even wrote it. I'll try editing it and send you the result later.

These words are trying to say that, some number of global objects can use a comdat key. If the object file they are contained in has the prevailing definition of that comdat key, then they will be included, and they will be discarded if not. The prevailing object is typically just the first object file that uses the comdat.

So, multiple objects from *one TU* can use the key and end up in the final binary, but objects from different TUs will never end up mixed together in the final binary.

[xur]

lgtm.

BTW, I'm in the process of committing D54175. After that patch, PGO instrumentation can be called after the main inlining. I don't think it will conflict anything in this patch

[rnk]

In D58737#1412847, @xur wrote:

lgtm.

BTW, I'm in the process of committing D54175. After that patch, PGO instrumentation can be called after the main inlining. I don't think it will conflict anything in this patch

I see, I guess I'll update the comment to say "may run before the inliner".

[vsk]

In D58737#1412846, @rnk wrote:

Oops, forgot to respond to this...

In D58737#1412734, @vsk wrote:

I'm confused by this wording re: comdats in the LangRef: "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key". Why can multiple global objects with the same comdat key end up in the final object file?

These words are trying to say that, some number of global objects can use a comdat key. If the object file they are contained in has the prevailing definition of that comdat key, then they will be included, and they will be discarded if not. The prevailing object is typically just the first object file that uses the comdat.

So, multiple objects from *one TU* can use the key and end up in the final binary, but objects from different TUs will never end up mixed together in the final binary.

Ah, got it, thanks :)

clang/test/Profile/cxx-templates.cpp

 // Tests for instrumentation of templated code. Each instantiation of a template
 // should be instrumented separately.
 
 // RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument=clang > %tgen
 // RUN: FileCheck --input-file=%tgen -check-prefix=T0GEN -check-prefix=ALL %s
 // RUN: FileCheck --input-file=%tgen -check-prefix=T100GEN -check-prefix=ALL %s
 
 // RUN: llvm-profdata merge %S/Inputs/cxx-templates.proftext -o %t.profdata
 // RUN: %clang_cc1 -x c++ %s -triple %itanium_abi_triple -main-file-name cxx-templates.cpp -std=c++11 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata > %tuse
 // RUN: FileCheck --input-file=%tuse -check-prefix=T0USE -check-prefix=ALL %s
 // RUN: FileCheck --input-file=%tuse -check-prefix=T100USE -check-prefix=ALL %s
 
-// T0GEN: @[[T0C:__profc__Z4loopILj0EEvv]] = {{(linkonce_odr hidden|internal)}} global [2 x i64] zeroinitializer
-// T100GEN: @[[T100C:__profc__Z4loopILj100EEvv]] = {{(linkonce_odr hidden|internal)}} global [2 x i64] zeroinitializer
+// T0GEN: @[[T0C:__profc__Z4loopILj0EEvv]] = linkonce_odr {{(hidden|dso_local)}} global [2 x i64] zeroinitializer
+// T100GEN: @[[T100C:__profc__Z4loopILj100EEvv]] = linkonce_odr {{(hidden|dso_local)}} global [2 x i64] zeroinitializer
 
 // T0GEN-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj0EEvv()
 // T0USE-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj0EEvv()
 // T100GEN-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj100EEvv()
 // T100USE-LABEL: define linkonce_odr {{.*}}void @_Z4loopILj100EEvv()
 template <unsigned N> void loop() {
   // ALL-NOT: ret
   // T0GEN: store {{.*}} @[[T0C]], i64 0, i64 0

compiler-rt/test/profile/coverage-inline.cpp

+// Test that the instrumentation puts the right linkage on the profile data for
+// inline functions.
 // RUN: %clang_profgen -g -fcoverage-mapping -c -o %t1.o %s -DOBJECT_1
 // RUN: %clang_profgen -g -fcoverage-mapping -c -o %t2.o %s
 // RUN: %clang_profgen -g -fcoverage-mapping %t1.o %t2.o -o %t.exe
 // RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t.exe
 // RUN: llvm-profdata show %t.profraw -all-functions | FileCheck %s
 
-// Test that the instrumentation puts the right linkage on the profile data for
-// inline functions.
+// Again, with optimizations and inlining. This tests that we use comdats
+// correctly.
+// RUN: %clang_profgen -O2 -g -fcoverage-mapping -c -o %t1.o %s -DOBJECT_1
+// RUN: %clang_profgen -O2 -g -fcoverage-mapping -c -o %t2.o %s
+// RUN: %clang_profgen -g -fcoverage-mapping %t1.o %t2.o -o %t.exe
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t.exe
+// RUN: llvm-profdata show %t.profraw -all-functions | FileCheck %s
 
 // CHECK:  {{.*}}foo{{.*}}:
 // CHECK-NEXT:    Hash:
 // CHECK-NEXT:    Counters: 1
 // CHECK-NEXT:    Function count: 1
 // CHECK:  {{.*}}inline_wrapper{{.*}}:
 // CHECK-NEXT:    Hash:
 // CHECK-NEXT:    Counters: 1

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
   if (TT.isOSBinFormatCOFF()) {
     Linkage = GlobalValue::InternalLinkage;
     Visibility = GlobalValue::DefaultVisibility;
   }
 
   // Move the name variable to the right section. Place them in a COMDAT group
   // if the associated function is a COMDAT. This will make sure that only one
-  // copy of counters of the COMDAT function will be emitted after linking.
+  // copy of counters of the COMDAT function will be emitted after linking. Keep
+  // in mind that this pass runs 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();
     if (TT.isOSBinFormatCOFF()) {
-      // There are two cases that need a comdat on COFF:
-      // 1. Functions that already have comdats (standard case)
-      // 2. available_externally functions (dllimport and C99 inline)
-      // In the first case, put all the data in the original function comdat. In
-      // the second case, create a new comdat group using the counter as the
-      // leader. It's linkage must be external, so use linkonce_odr linkage in
-      // that case.
-      if (Comdat *C = Fn->getComdat()) {
-        Cmdt = C;
-      } else {
-        Cmdt = M->getOrInsertComdat(
-            getVarName(Inc, getInstrProfCountersVarPrefix()));
-        CounterLinkage = GlobalValue::LinkOnceODRLinkage;
-      }
-    } else {
-      // For other platforms that use comdats (ELF), make a new comdat group for
-      // all the profile data. It will be deduplicated within the current DSO.
-      Cmdt = M->getOrInsertComdat(getVarName(Inc, getInstrProfComdatPrefix()));
-    }
+      // 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();
+    Cmdt = M->getOrInsertComdat(getVarName(Inc, CmdtPrefix));
   }
 
   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-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: @__llvm_prf_nm = private constant [{{.*}} x i8] c"{{.*}}", section "{{.*}}__llvm_prf_names"
 
 
 ; COFF-NOT: __profn__Z3barIvEvv
-; 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
+; 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
 
 
 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

 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 = internal global{{.*}}, section ".lprfc$M", comdat($foo_inline), align 8
-; COFF: @__profd_foo_inline = internal global{{.*}}, section ".lprfd$M", comdat($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
 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"