[InstrProf] Disable marking COMDAT profd variable as private linkage

[xur]

Summary

It is not safe to set private linkage for the profd variable created for the COMDAT function. For the LTO build where the COMDAT group might be dissolved. If that happens, we will have multiple "profd" in the final binary and result in multiple function counters for the same function.

This fixes PR58215: https://github.com/llvm/llvm-project/issues/58215.

[MaskRay]

I agree that the current setup is problematic for ThinLTO. I think we can retain the current size optimization of private profd (suppressing the symbol table entry). D135427 tries to fix this on the FunctionImport side.
clang -O2 -fuse-ld=lld -fprofile-generate=./t m1.o m2.o -flto=thin -o t_gen will work.

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

#include "llvm/ProfileData/InstrProfData.inc"
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
     Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);
 
   // If the data variable is not referenced by code (if we don't emit
   // @llvm.instrprof.value.profile, NS will be 0), and the counter keeps the
   // data variable live under linker GC, the data variable can be private. This
   // optimization applies to ELF.
   //
-  // On COFF, a comdat leader cannot be local so we require DataReferencedByCode
-  // to be false.
-  //
-  // If profd is in a deduplicate comdat, NS==0 with a hash suffix guarantees
-  // that other copies must have the same CFG and cannot have value profiling.
-  // If no hash suffix, other profd copies may be referenced by code.
-  if (NS == 0 && !(DataReferencedByCode && NeedComdat && !Renamed) &&
+  // We cannot set profd to private if the assoicated function is a COMDAT:
+  // LTO might dissolve the COMDAT group, resulting duplicated profd and
+  // duplicated function entries in the profile.
+  if (NS == 0 && !NeedComdat &&
       (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

llvm/test/Instrumentation/InstrProfiling/comdat.ll

 $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
+; ELF0:  @__profd_foo_inline = linkonce_odr hidden 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 = internal global {{.*}}, section ".lprfd$M", comdat($__profc_foo_inline), align 8
+; COFF0: @__profd_foo_inline = linkonce_odr hidden 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
+; ELF0:  @__profd_foo_extern = linkonce_odr hidden 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 = internal global{{.*}}, section ".lprfd$M", comdat($__profc_foo_extern), align 8
+; COFF0: @__profd_foo_extern = linkonce_odr hidden 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:   @__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

llvm/test/Instrumentation/InstrProfiling/profiling.ll

 ; XCOFF: @__profc_foo_inline = private global
 ; XCOFF: @__profd_foo_inline = private global
 define linkonce_odr void @foo_inline() {
   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
-; ELF: @__profd_foo_extern = private global {{.*}}section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
+; ELF: @__profd_foo_extern = linkonce_odr hidden 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 = internal global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
+; COFF: @__profd_foo_extern = linkonce_odr hidden 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)

llvm/test/Transforms/PGOProfile/comdat.ll

 ;; Test comdat functions.
 ; RUN: opt < %s -mtriple=x86_64-linux -passes=pgo-instr-gen,instrprof -S | FileCheck %s --check-prefixes=ELF
 ; RUN: opt < %s -mtriple=x86_64-windows -passes=pgo-instr-gen,instrprof -S | FileCheck %s --check-prefixes=COFF
 
 $linkonceodr = comdat any
 $weakodr = comdat any
 
 ;; profc/profd have hash suffixes. This definition doesn't have value profiling,
 ;; so definitions with the same name in other modules must have the same CFG and
 ;; cannot have value profiling, either. profd can be made private for ELF.
 ; ELF:   @__profc_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
-; ELF:   @__profd_linkonceodr.[[#]] = private global {{.*}} comdat($__profc_linkonceodr.[[#]]), align 8
+; ELF:   @__profd_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat($__profc_linkonceodr.[[#]]), align 8
 ; COFF:  @__profc_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
 ; COFF:  @__profd_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
 define linkonce_odr void @linkonceodr() comdat {
   ret void
 }
 
 ;; weakodr in a comdat is not renamed. There is no guarantee that definitions in
 ;; other modules don't have value profiling. profd should be conservatively

llvm/test/Transforms/PGOProfile/indirect_call_profile.ll

 
 @bar = external global ptr, align 8
 
 ; GEN: @__profn_novp_inline = linkonce_odr hidden constant [11 x i8] c"novp_inline"
 ; GEN: @__profn_foo = private constant [3 x i8] c"foo"
 ; GEN: @__profn_vp_inline = linkonce_odr hidden constant [9 x i8] c"vp_inline"
 
 ;; Test that a linkonce function's address is recorded.
-;; We allow a linkonce profd to be private if the function does not use value profiling.
-; LOWER:      @__profd_novp_inline.[[HASH:[0-9]+]] = private global {{.*}} @__profc_novp_inline.[[HASH]]
+; LOWER:      @__profd_novp_inline.[[HASH:[0-9]+]] = linkonce_odr hidden global {{.*}} @__profc_novp_inline.[[HASH]]
 ; LOWER-SAME:   ptr @novp_inline
 ; LOWER:      @__profd_foo = private {{.*}} @__profc_foo
 
 ;; __profd_vp_inline.[[#]] is referenced by code and may be referenced by other
 ;; text sections due to inlining. It can't be local because a linker error would
 ;; occur if a prevailing text section references the non-prevailing local symbol.
 ; LOWER:      @__profd_vp_inline.[[FOO_HASH:[0-9]+]] = linkonce_odr hidden {{.*}} @__profc_vp_inline.[[FOO_HASH]]
 ; LOWER-SAME:   ptr @vp_inline