Put profile variables of COMDAT functions to it's own COMDAT group

[xur]

Summary

In -fprofile-instr-generate compilation, to remove the redundant profile variables for the COMDAT functions, these variables are placed in the same COMDAT group as its associated function.
This way when the COMDAT function is not picked by the linker, those profile variables will also not be output in the final binary.

One problem here is that we lose the ability to link the objects built w and w/o -fprofile-instr-generate. The linker may choose one version of COMDAT that not expanded (i.e. not built with -fprofile-instr-generate). All the references to the variables
become dangling. The result binary will segfalut. If ld.gold is used, we usually see a warning like:
foo.o:foo.bc:function __llvm_profile_register_functions: warning: relocation refers to discarded section

This patch puts the profile variables for COMDAT functions to its
own COMDAT group to avoid the problem.

[davidxl]

InstrProfiling.cpp

206 ↗

(On Diff #32850)

How about "var" --> "vars" to indicate the comdat section includes _counters, _data, _name ?

[dnovillo]

With David's suggestions and a test case, this LGTM.

InstrProfiling.cpp

199 ↗

(On Diff #32850)

s/Make/Place/

[xur]

The new patch integrated David and Diego's comments.
I found that there is an existing test case that can test the new behavior. So I will not add a new test case.

llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp

 
 GlobalVariable *
 InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   GlobalVariable *Name = Inc->getName();
   auto It = RegionCounters.find(Name);
   if (It != RegionCounters.end())
     return It->second;
 
-  // Move the name variable to the right section. Make sure it is placed in the
-  // same comdat as its associated function. Otherwise, we may get multiple
-  // counters for the same function in certain cases.
+  // 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.
   Function *Fn = Inc->getParent()->getParent();
+  Comdat *ProfileVarsComdat = nullptr;
+  if (Fn->hasComdat())
+    ProfileVarsComdat = M->getOrInsertComdat(StringRef(getVarName(Inc, "vars")));
   Name->setSection(getNameSection());
   Name->setAlignment(1);
-  Name->setComdat(Fn->getComdat());
+  Name->setComdat(ProfileVarsComdat);
 
   uint64_t NumCounters = Inc->getNumCounters()->getZExtValue();
   LLVMContext &Ctx = M->getContext();
   ArrayType *CounterTy = ArrayType::get(Type::getInt64Ty(Ctx), NumCounters);
 
   // Create the counters variable.
   auto *Counters = new GlobalVariable(*M, CounterTy, false, Name->getLinkage(),
                                       Constant::getNullValue(CounterTy),
                                       getVarName(Inc, "counters"));
   Counters->setVisibility(Name->getVisibility());
   Counters->setSection(getCountersSection());
   Counters->setAlignment(8);
-  Counters->setComdat(Fn->getComdat());
+  Counters->setComdat(ProfileVarsComdat);
 
   RegionCounters[Inc->getName()] = Counters;
 
   // Create data variable.
   auto *NameArrayTy = Name->getType()->getPointerElementType();
   auto *Int32Ty = Type::getInt32Ty(Ctx);
   auto *Int64Ty = Type::getInt64Ty(Ctx);
   auto *Int8PtrTy = Type::getInt8PtrTy(Ctx);
   auto *Int64PtrTy = Type::getInt64PtrTy(Ctx);
 
   Type *DataTypes[] = {Int32Ty, Int32Ty, Int64Ty, Int8PtrTy, Int64PtrTy};
   auto *DataTy = StructType::get(Ctx, makeArrayRef(DataTypes));
   Constant *DataVals[] = {
       ConstantInt::get(Int32Ty, NameArrayTy->getArrayNumElements()),
       ConstantInt::get(Int32Ty, NumCounters),
       ConstantInt::get(Int64Ty, Inc->getHash()->getZExtValue()),
       ConstantExpr::getBitCast(Name, Int8PtrTy),
       ConstantExpr::getBitCast(Counters, Int64PtrTy)};
   auto *Data = new GlobalVariable(*M, DataTy, true, Name->getLinkage(),
                                   ConstantStruct::get(DataTy, DataVals),
                                   getVarName(Inc, "data"));
   Data->setVisibility(Name->getVisibility());
   Data->setSection(getDataSection());
   Data->setAlignment(8);
-  Data->setComdat(Fn->getComdat());
+  Data->setComdat(ProfileVarsComdat);
 
   // Mark the data variable as used so that it isn't stripped out.
   UsedVars.push_back(Data);
 
   return Counters;
 }
 
 void InstrProfiling::emitRegistration() {

llvm/trunk/test/Instrumentation/InstrProfiling/PR23499.ll

 ;; Check that data associated with linkonce odr functions are placed in
 ;; the same comdat section as their associated function.
 
 ; RUN: opt < %s -mtriple=x86_64-apple-macosx10.10.0 -instrprof -S | FileCheck %s
 ; RUN: opt < %s -mtriple=x86_64-unknown-linux -instrprof -S | FileCheck %s
 
 $_Z3barIvEvv = comdat any
 
 @__llvm_profile_name__Z3barIvEvv = linkonce_odr hidden constant [11 x i8] c"_Z3barIvEvv", align 1
 
-; CHECK: @__llvm_profile_name__Z3barIvEvv = linkonce_odr hidden constant [11 x i8] c"_Z3barIvEvv", section "{{.*}}__llvm_prf_names", comdat($_Z3barIvEvv), align 1
-; CHECK: @__llvm_profile_counters__Z3barIvEvv = linkonce_odr hidden global [1 x i64] zeroinitializer, section "{{.*}}__llvm_prf_cnts", comdat($_Z3barIvEvv), align 8
-; CHECK: @__llvm_profile_data__Z3barIvEvv = linkonce_odr hidden constant { i32, i32, i64, i8*, i64* } { i32 11, i32 1, i64 0, i8* getelementptr inbounds ([11 x i8], [11 x i8]* @__llvm_profile_name__Z3barIvEvv, i32 0, i32 0), i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__llvm_profile_counters__Z3barIvEvv, i32 0, i32 0) }, section "{{.*}}__llvm_prf_data", comdat($_Z3barIvEvv), align 8
+; CHECK: @__llvm_profile_name__Z3barIvEvv = linkonce_odr hidden constant [11 x i8] c"_Z3barIvEvv", section "{{.*}}__llvm_prf_names", comdat($__llvm_profile_vars__Z3barIvEvv), align 1
+; CHECK: @__llvm_profile_counters__Z3barIvEvv = linkonce_odr hidden global [1 x i64] zeroinitializer, section "{{.*}}__llvm_prf_cnts", comdat($__llvm_profile_vars__Z3barIvEvv), align 8
+; CHECK: @__llvm_profile_data__Z3barIvEvv = linkonce_odr hidden constant { i32, i32, i64, i8*, i64* } { i32 11, i32 1, i64 0, i8* getelementptr inbounds ([11 x i8], [11 x i8]* @__llvm_profile_name__Z3barIvEvv, i32 0, i32 0), i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__llvm_profile_counters__Z3barIvEvv, i32 0, i32 0) }, section "{{.*}}__llvm_prf_data", comdat($__llvm_profile_vars__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]* @__llvm_profile_name__Z3barIvEvv, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }