[llvm] 32e2326 - Revert D108432 "[InstrProfiling] Keep profd non-private for non-renamable comdat functions"

[Fangrui Song via llvm-commits]

Author: Fangrui Song
Date: 2021-08-24T19:16:07-07:00
New Revision: 32e2326cda441528f75404b1c93374722caaf21c

URL: https://github.com/llvm/llvm-project/commit/32e2326cda441528f75404b1c93374722caaf21c
DIFF: https://github.com/llvm/llvm-project/commit/32e2326cda441528f75404b1c93374722caaf21c.diff

LOG: Revert D108432 "[InstrProfiling] Keep profd non-private for non-renamable comdat functions"

This reverts commit f653beea88d2684cdc8117e662b321ba04666771.

It broke Windows coverage-inline.cpp because link.exe has a limitation
that external symbols in IMAGE_COMDAT_SELECT_ASSOCIATIVE don't work.

It essentially dropped the previous size optimization for coverage
because coverage doesn't rename comdat by default.
Needs more investigation what we should do.

Added: 
    

Modified: 
    llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
    llvm/test/Instrumentation/InstrProfiling/linkage.ll
    llvm/test/Transforms/PGOProfile/comdat.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index aa383b71b40c7..d2620fd593e15 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -758,18 +758,14 @@ void InstrProfiling::lowerCoverageData(GlobalVariable *CoverageNamesVar) {
 }
 
 /// Get the name of a profiling variable for a particular function.
-static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix,
-                              bool &Renamed) {
+static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
   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)) {
-    Renamed = false;
+      !canRenameComdatFunc(*F))
     return (Prefix + Name).str();
-  }
-  Renamed = true;
   uint64_t FuncHash = Inc->getHash()->getZExtValue();
   SmallVector<char, 24> HashPostfix;
   if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix)))
@@ -882,11 +878,8 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   // discarded.
   bool DataReferencedByCode = profDataReferencedByCode(*M);
   bool NeedComdat = needsComdatForCounter(*Fn, *M);
-  bool Renamed;
-  std::string CntsVarName =
-      getVarName(Inc, getInstrProfCountersVarPrefix(), Renamed);
-  std::string DataVarName =
-      getVarName(Inc, getInstrProfDataVarPrefix(), Renamed);
+  std::string CntsVarName = getVarName(Inc, getInstrProfCountersVarPrefix());
+  std::string DataVarName = getVarName(Inc, getInstrProfDataVarPrefix());
   auto MaybeSetComdat = [&](GlobalVariable *GV) {
     bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
     if (UseComdat) {
@@ -927,7 +920,7 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
     ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS);
     auto *ValuesVar = new GlobalVariable(
         *M, ValuesTy, false, Linkage, Constant::getNullValue(ValuesTy),
-        getVarName(Inc, getInstrProfValuesVarPrefix(), Renamed));
+        getVarName(Inc, getInstrProfValuesVarPrefix()));
     ValuesVar->setVisibility(Visibility);
     ValuesVar->setSection(
         getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
@@ -962,13 +955,8 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
   //
   // 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 && !(NeedComdat && !Renamed) &&
-      (TT.isOSBinFormatELF() ||
-       (!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
+  if (NS == 0 && (TT.isOSBinFormatELF() ||
+                  (!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
     Linkage = GlobalValue::PrivateLinkage;
     Visibility = GlobalValue::DefaultVisibility;
   }

diff  --git a/llvm/test/Instrumentation/InstrProfiling/linkage.ll b/llvm/test/Instrumentation/InstrProfiling/linkage.ll
index bb7f33f9deb5d..bb381afeafbd2 100644
--- a/llvm/test/Instrumentation/InstrProfiling/linkage.ll
+++ b/llvm/test/Instrumentation/InstrProfiling/linkage.ll
@@ -70,11 +70,11 @@ define linkonce_odr void @foo_inline() {
 }
 
 ; ELF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_cnts", comdat, align 8
-; ELF: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_data", comdat($__profc_foo_extern), 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 = linkonce_odr hidden global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
+; COFF: @__profd_foo_extern = private 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

diff  --git a/llvm/test/Transforms/PGOProfile/comdat.ll b/llvm/test/Transforms/PGOProfile/comdat.ll
index 9f5c0ee848ca5..6a9720365f3ed 100644
--- a/llvm/test/Transforms/PGOProfile/comdat.ll
+++ b/llvm/test/Transforms/PGOProfile/comdat.ll
@@ -16,12 +16,9 @@ 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.
+;; weakodr in a comdat is not renamed.
 ; ELF:   @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
-; ELF:   @__profd_weakodr = weak_odr hidden global {{.*}} comdat($__profc_weakodr), align 8
+; ELF:   @__profd_weakodr = private 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 {