[InstrProfiling] Keep profd non-private for non-renamable comdat functions

[MaskRay]

Summary

The NS==0 condition used by D103717 missed a corner case:
if the current copy does not have a hash suffix (e.g. weak_odr), a copy
with value profiling (with a different CFG) may exist. This is super
rare, but is possible with regular PGO (which can make a weak_odr function
inlines its callees differently, sometimes with value profiling while
sometimes without).

If the current copy with private profd is prevailing, the non-prevailing
copy may get an undefined symbol if a caller inlining the non-prevailing
function references its profd. If the other copy with non-private profd
is prevailing, the current copy may cause a "relocation to discarded
section" linker error.

The fix is straightforward: just keep non-private profd in this case.

With this change, a stage 2 (-DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_BUILD_INSTRUMENTED=IR)
clang is 0.08% larger (172431496/172286720-1).
`stat -c %s **/*.o | awk '{s+=$1}END{print s}' is 0.026% larger.
The majority of D103717's benefits remains.

[MaskRay]

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

965–971

canRenameComdatFunc should check whether the variable is actually renamed.

[MaskRay]

..

[MaskRay]

.

[YolandaCY]

"This is super rare, but is possible with CSPGO (which can make a weak_odr function inlines its callees differently, sometimes with value profiling while sometimes without)."

This may not be an issue to CSPGO, if the IRPGOFlag can be correctly handled as addressed at https://reviews.llvm.org/D107034.
So this may only happen with -fprofile-instr-generate, which should be in a rare case (not sure if it's possible), but not specific to CSPGO.

[davidxl]

This looks reasonable. How much size impact does this patch have?

[MaskRay]

In D108432#2958278, @davidxl wrote:

This looks reasonable. How much size impact does this patch have?

With this change, a stage 2 (-DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_BUILD_INSTRUMENTED=IR)
clang is 0.08% larger (172431496/172286720-1).
`stat -c %s **/*.o | awk '{s+=$1}END{print s}' is 0.026% larger.
The majority of D103717's benefits remains.

The additional __profd_ symbols are due to weak_odr functions, which are mostly explicit template instantiations.

[MaskRay]

In D108432#2956919, @YolandaCY wrote:

"This is super rare, but is possible with CSPGO (which can make a weak_odr function inlines its callees differently, sometimes with value profiling while sometimes without)."

This may not be an issue to CSPGO, if the IRPGOFlag can be correctly handled as addressed at https://reviews.llvm.org/D107034.
So this may only happen with -fprofile-instr-generate, which should be in a rare case (not sure if it's possible), but not specific to CSPGO.

Replied there. I think isIRPGOFlagSet(M) => enablesValueProfiling(*M) is not correct.

[xur]

The problem you described is exactly what we wanted to solve with comdat renaming.
We had some correctness issues so I put many restrictions here. We have switched the default linker to lld
and linkage handling has evolved quite bit. I might revisit some of
the restrictions again, like if we can enable the renaming for weak linkage.

The problem can happen for regular PGO. As for CSPGO, the instrumentation is post inline
so the prevailing selection happens before instrumentation. The non-prevailing copy is already
dropped. I don't this will happen in CSPGO.

Anyway, being conservative is reasonable here. This looks good to me.

[vitalybuka]

Windows is broken https://lab.llvm.org/buildbot/#/builders/127

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

for (unsigned I = 0, E = Names->getNumOperands(); I < E; ++I) {
     Name->setLinkage(GlobalValue::PrivateLinkage);
     ReferencedNames.push_back(Name);
     NC->dropAllReferences();
   }
   CoverageNamesVar->eraseFromParent();
 }
 
 /// Get the name of a profiling variable for a particular function.
-static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
+static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix,
+                              bool &Renamed) {
   StringRef NamePrefix = getInstrProfNameVarPrefix();
   StringRef Name = Inc->getName()->getName().substr(NamePrefix.size());
   Function *F = Inc->getParent()->getParent();
   Module *M = F->getParent();
   if (!DoHashBasedCounterSplit || !isIRPGOFlagSet(M) ||
-      !canRenameComdatFunc(*F))
+      !canRenameComdatFunc(*F)) {
+    Renamed = false;
     return (Prefix + Name).str();
+  }
+  Renamed = true;
   uint64_t FuncHash = Inc->getHash()->getZExtValue();
   SmallVector<char, 24> HashPostfix;
   if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix)))
     return (Prefix + Name).str();
   return (Prefix + Name + "." + Twine(FuncHash)).str();
 }
 
 static uint64_t getIntModuleFlagOrZero(const Module &M, StringRef Flag) {
InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   // same name marked IMAGE_COMDAT_SELECT_ASSOCIATIVE.
   //
   // For ELF, when not using COMDAT, put counters, data and values into a
   // nodeduplicate COMDAT which is lowered to a zero-flag section group. This
   // allows -z start-stop-gc to discard the entire group when the function is
   // discarded.
   bool DataReferencedByCode = profDataReferencedByCode(*M);
   bool NeedComdat = needsComdatForCounter(*Fn, *M);
-  std::string CntsVarName = getVarName(Inc, getInstrProfCountersVarPrefix());
-  std::string DataVarName = getVarName(Inc, getInstrProfDataVarPrefix());
+  bool Renamed;
+  std::string CntsVarName =
+      getVarName(Inc, getInstrProfCountersVarPrefix(), Renamed);
+  std::string DataVarName =
+      getVarName(Inc, getInstrProfDataVarPrefix(), Renamed);
   auto MaybeSetComdat = [&](GlobalVariable *GV) {
     bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
     if (UseComdat) {
       StringRef GroupName = TT.isOSBinFormatCOFF() && DataReferencedByCode
                                 ? GV->getName()
                                 : CntsVarName;
       Comdat *C = M->getOrInsertComdat(GroupName);
       if (!NeedComdat)
InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   uint64_t NS = 0;
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
     NS += PD.NumValueSites[Kind];
   if (NS > 0 && ValueProfileStaticAlloc &&
       !needsRuntimeRegistrationOfSectionRange(TT)) {
     ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS);
     auto *ValuesVar = new GlobalVariable(
         *M, ValuesTy, false, Linkage, Constant::getNullValue(ValuesTy),
-        getVarName(Inc, getInstrProfValuesVarPrefix()));
+        getVarName(Inc, getInstrProfValuesVarPrefix(), Renamed));
     ValuesVar->setVisibility(Visibility);
     ValuesVar->setSection(
         getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
     ValuesVar->setAlignment(Align(8));
     MaybeSetComdat(ValuesVar);
     ValuesPtrExpr =
         ConstantExpr::getBitCast(ValuesVar, Type::getInt8PtrTy(Ctx));
   }
#include "llvm/ProfileData/InstrProfData.inc"
 
   // 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 (NS == 0 && (TT.isOSBinFormatELF() ||
-                  (!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
+  //
+  // 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 && !(NeedComdat && !Renamed) &&
+      (TT.isOSBinFormatELF() ||
     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 =

llvm/test/Instrumentation/InstrProfiling/linkage.ll

 ; COFF: @__profc_foo_inline = linkonce_odr hidden global{{.*}} section ".lprfc$M", align 8
 ; COFF: @__profd_foo_inline = private global{{.*}} section ".lprfd$M", 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
 }
 
 ; 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 = private 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
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
 }
 
 declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
 
 ; MACHO: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} {
 ; MACHO:   %[[REG:.*]] = load i32, i32* @__llvm_profile_runtime
 ; MACHO:   ret i32 %[[REG]]
 ; MACHO: }
 ; COFF: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} comdat {
 ; ELF-NOT: define linkonce_odr hidden i32 @__llvm_profile_runtime_user() {{.*}} {
 ; ELF-NOT:   %[[REG:.*]] = load i32, i32* @__llvm_profile_runtime

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
+; 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
+;; non-private to prevent a caller from referencing a non-prevailing profd,
+;; causing a linker error.
+; ELF:   @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
+; ELF:   @__profd_weakodr = weak_odr hidden global {{.*}} comdat($__profc_weakodr), align 8
+; COFF:  @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
+; COFF:  @__profd_weakodr = weak_odr hidden global {{.*}} comdat, align 8
+define weak_odr void @weakodr() comdat {
+  ret void
+}