[InstrProf] Set prof global variables to internal linkage if adding a comdat

[aeubanks]

Summary

COFF has a verifier check that private global variables don't have a comdat of the same name.

[aeubanks]

I'm not sure if I'm understanding the intent of the verifier check correctly. Is it that all private GVs on COFF shouldn't have a comdat, or they specifically shouldn't have a comdat of the same name?

[rnk]

I don't think this current revision will work, I don't imagine you would be able to build Chrome for Windows with coverage with this change, or if you did, the binary and object file sizes would be much larger.

llvm/test/Instrumentation/InstrProfiling/comdat.ll

33

This seems like an interesting behavior change. Why do we want this? The intention of the current structure is that the profd data reuses the profc comdat, so we don't need to include profd in the symbol table. I believe I did this to reduce object file size and symbol table size. If we don't use a comdat here, then we will end up with extra duplicate profd data that we don't want.

60

It sounds like the real corner case here is doing coverage for functions with private linkage. To handle that case, I think we should make the __profc data internal linkage instead of private. Internal linkage will produce a symbol table entry which can be used with comdats.

[aeubanks]

update

[aeubanks]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

1031

linkage is already set here

[rnk]

lgtm

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

917–918

I would phase this differently: COFF doesn't allow the comdat group leader to have private linkage, so upgrade private linkage to internal linkage to produce a symbol table entry.

That's the difference between private and internal linkage, at least for ELF and COFF: whether or not there is a symbol table entry. COFF comdat info lives in the symbol table, so if you don't have one, you can't be a comdat symbol.

I think the next best alternative fix would be to paper over this platform specific detail in the backend, and produce symbol table entries for private+comdat symbols. I don't recall why the verifier error was preferred over this.

[MaskRay]

LGTM

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

auto MaybeSetComdat = [&](GlobalVariable *GV) {
     if (UseComdat) {
       StringRef GroupName = TT.isOSBinFormatCOFF() && DataReferencedByCode
                                 ? GV->getName()
                                 : CntsVarName;
       Comdat *C = M->getOrInsertComdat(GroupName);
       if (!NeedComdat)
         C->setSelectionKind(Comdat::NoDeduplicate);
       GV->setComdat(C);
+      // COFF doesn't allow the comdat group leader to have private linkage, so
+      // upgrade private linkage to internal linkage to produce a symbol table
+      // entry.
+      if (TT.isOSBinFormatCOFF() && GV->hasPrivateLinkage())
+        GV->setLinkage(GlobalValue::InternalLinkage);
     }
   };
 
   uint64_t NumCounters = Inc->getNumCounters()->getZExtValue();
   LLVMContext &Ctx = M->getContext();
 
   auto *CounterPtr = createRegionCounters(Inc, CntsVarName, Linkage);
   CounterPtr->setVisibility(Visibility);
   CounterPtr->setSection(
       getInstrProfSectionName(IPSK_cnts, TT.getObjectFormat()));
-  MaybeSetComdat(CounterPtr);
   CounterPtr->setLinkage(Linkage);
+  MaybeSetComdat(CounterPtr);
   PD.RegionCounters = CounterPtr;
   if (DebugInfoCorrelate) {
     if (auto *SP = Fn->getSubprogram()) {
       DIBuilder DB(*M, true, SP->getUnit());
       Metadata *FunctionNameAnnotation[] = {
           MDString::get(Ctx, InstrProfCorrelator::FunctionNameAttributeName),
           MDString::get(Ctx, getPGOFuncNameVarInitializer(NamePtr)),
       };
#include "llvm/ProfileData/InstrProfData.inc"
   // If no hash suffix, other profd copies may be referenced by code.
   if (NS == 0 && !(DataReferencedByCode && NeedComdat && !Renamed) &&
       (TT.isOSBinFormatELF() ||
        (!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
     Linkage = GlobalValue::PrivateLinkage;
     Visibility = GlobalValue::DefaultVisibility;
   }
   auto *Data =
       new GlobalVariable(*M, DataTy, false, Linkage, nullptr, DataVarName);
   // Reference the counter variable with a label difference (link-time
   // constant).
   auto *RelativeCounterPtr =
       ConstantExpr::getSub(ConstantExpr::getPtrToInt(CounterPtr, IntPtrTy),
                            ConstantExpr::getPtrToInt(Data, IntPtrTy));
 
   Constant *DataVals[] = {
 #define INSTR_PROF_DATA(Type, LLVMType, Name, Init) Init,
 #include "llvm/ProfileData/InstrProfData.inc"
   };
   Data->setInitializer(ConstantStruct::get(DataTy, DataVals));
 
   Data->setVisibility(Visibility);
   Data->setSection(getInstrProfSectionName(IPSK_data, TT.getObjectFormat()));
   Data->setAlignment(Align(INSTR_PROF_DATA_ALIGNMENT));
   MaybeSetComdat(Data);
-  Data->setLinkage(Linkage);
 
   PD.DataVar = Data;
 
   // Mark the data variable as used so that it isn't stripped out.
   CompilerUsedVars.push_back(Data);
   // Now that the linkage set by the FE has been passed to the data and counter
   // variables, reset Name variable's linkage and visibility to private so that
   // it can be removed later by the compiler.

llvm/test/Instrumentation/InstrProfiling/comdat.ll

 ; RUN: opt < %t0.ll -mtriple=x86_64-windows -passes=instrprof -S | FileCheck %s --check-prefixes=COFF,COFF0
 ; RUN: opt < %t1.ll -mtriple=x86_64-windows -passes=instrprof -S | FileCheck %s --check-prefixes=COFF,COFF1
 
 ;--- main.ll
 declare void @llvm.instrprof.increment(ptr, i64, i32, i32)
 
 $foo_inline = comdat any
 $foo_extern = comdat any
+$foo_private_profn = comdat any
 
 @__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"
+@__profn_foo_private_profn = private constant [17 x i8] c"foo_private_profn"
 
 ;; When value profiling is disabled, __profd_ variables are not referenced by
 ;; code. We can use private linkage. When enabled, __profd_ needs to be
 ;; non-local which requires separate comdat on COFF due to a link.exe limitation.
 
 ; ELF:   @__profc_foo_inline = linkonce_odr hidden global {{.*}}, section "__llvm_prf_cnts", comdat, align 8
 ; ELF0:  @__profd_foo_inline = private global {{.*}}, section "__llvm_prf_data", comdat($__profc_foo_inline), align 8
 ; ELF1:  @__profd_foo_inline = linkonce_odr hidden global {{.*}}, section "__llvm_prf_data", comdat($__profc_foo_inline), align 8
 ; COFF0: @__profc_foo_inline = linkonce_odr hidden global {{.*}}, section ".lprfc$M", comdat, align 8
-; COFF0: @__profd_foo_inline = private global {{.*}}, section ".lprfd$M", comdat($__profc_foo_inline), align 8
+; COFF0: @__profd_foo_inline = internal 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(ptr @__profn_foo_inline, i64 0, i32 1, i32 0)
   ret void
 }
 
 ; ELF:   @__profc_foo_extern = 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
-; COFF0: @__profd_foo_extern = private global{{.*}}, section ".lprfd$M", comdat($__profc_foo_extern), align 8
+; COFF0: @__profd_foo_extern = internal global{{.*}}, section ".lprfd$M", comdat($__profc_foo_extern), align 8
 ; COFF1: @__profd_foo_extern = linkonce_odr hidden global{{.*}}, section ".lprfd$M", comdat, align 8
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(ptr @__profn_foo_extern, i64 0, i32 1, i32 0)
   ret void
 }
 
-; ELF:   @llvm.compiler.used = appending global [2 x ptr] [ptr @__profd_foo_inline, ptr @__profd_foo_extern]
-; COFF0: @llvm.compiler.used = appending global [3 x ptr] [ptr @__llvm_profile_runtime_user, ptr @__profd_foo_inline, ptr @__profd_foo_extern]
-; COFF1: @llvm.used = appending global [4 x ptr] [ptr @__llvm_profile_runtime_user, ptr @__profd_foo_inline, ptr @__profd_foo_extern, ptr @__llvm_prf_nm]
+; ELF:   @__profc_foo_private_profn = private global{{.*}}, section "__llvm_prf_cnts", comdat, align 8
+; ELF0:  @__profd_foo_private_profn = private global{{.*}}, section "__llvm_prf_data", comdat($__profc_foo_private_profn), align 8
+; ELF1:  @__profd_foo_private_profn = private global{{.*}}, section "__llvm_prf_data", comdat($__profc_foo_private_profn), align 8
+; COFF:  @__profc_foo_private_profn = internal global{{.*}}, section ".lprfc$M", comdat, align 8
+; COFF0: @__profd_foo_private_profn = internal global{{.*}}, section ".lprfd$M", comdat($__profc_foo_private_profn), align 8
+; COFF1: @__profd_foo_private_profn = internal global{{.*}}, section ".lprfd$M", comdat, align 8
+define void @foo_private_profn() comdat {
+  call void @llvm.instrprof.increment(ptr @__profn_foo_private_profn, i64 0, i32 1, i32 0)
+  ret void
+}
+
+; ELF:   @llvm.compiler.used = appending global [3 x ptr] [ptr @__profd_foo_inline, ptr @__profd_foo_extern, ptr @__profd_foo_private_profn]
+; COFF0: @llvm.compiler.used = appending global [4 x ptr] [ptr @__llvm_profile_runtime_user, ptr @__profd_foo_inline, ptr @__profd_foo_extern, ptr @__profd_foo_private_profn]
+; COFF1: @llvm.used = appending global [5 x ptr] [ptr @__llvm_profile_runtime_user, ptr @__profd_foo_inline, ptr @__profd_foo_extern, ptr @__profd_foo_private_profn, ptr @__llvm_prf_nm]
 
 ;--- 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/Instrumentation/InstrProfiling/profiling.ll

define linkonce_odr void @foo_inline() {
   ret void
 }
 
 ; ELF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_cnts", comdat, align 8
 ; ELF: @__profd_foo_extern = private global {{.*}}section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
 ; MACHO: @__profc_foo_extern = linkonce_odr hidden global
 ; MACHO: @__profd_foo_extern = linkonce_odr hidden global
 ; COFF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfc$M", comdat, align 8
-; COFF: @__profd_foo_extern = private global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
+; COFF: @__profd_foo_extern = internal global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
 ; XCOFF: @__profc_foo_extern = private global
 ; XCOFF: @__profd_foo_extern = private global
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(ptr @__profn_foo_extern, i64 0, i32 1, i32 0)
   ret void
 }
 
 declare void @llvm.instrprof.increment(ptr, i64, i32, i32)