Skip to content

Commit 32e2326

Browse files
committed
Revert D108432 "[InstrProfiling] Keep profd non-private for non-renamable comdat functions"
This reverts commit f653bee. 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.
1 parent 5541a05 commit 32e2326

File tree

3 files changed

+11
-26
lines changed

3 files changed

+11
-26
lines changed

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -758,18 +758,14 @@ void InstrProfiling::lowerCoverageData(GlobalVariable *CoverageNamesVar) {
758758
}
759759

760760
/// Get the name of a profiling variable for a particular function.
761-
static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix,
762-
bool &Renamed) {
761+
static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
763762
StringRef NamePrefix = getInstrProfNameVarPrefix();
764763
StringRef Name = Inc->getName()->getName().substr(NamePrefix.size());
765764
Function *F = Inc->getParent()->getParent();
766765
Module *M = F->getParent();
767766
if (!DoHashBasedCounterSplit || !isIRPGOFlagSet(M) ||
768-
!canRenameComdatFunc(*F)) {
769-
Renamed = false;
767+
!canRenameComdatFunc(*F))
770768
return (Prefix + Name).str();
771-
}
772-
Renamed = true;
773769
uint64_t FuncHash = Inc->getHash()->getZExtValue();
774770
SmallVector<char, 24> HashPostfix;
775771
if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix)))
@@ -882,11 +878,8 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
882878
// discarded.
883879
bool DataReferencedByCode = profDataReferencedByCode(*M);
884880
bool NeedComdat = needsComdatForCounter(*Fn, *M);
885-
bool Renamed;
886-
std::string CntsVarName =
887-
getVarName(Inc, getInstrProfCountersVarPrefix(), Renamed);
888-
std::string DataVarName =
889-
getVarName(Inc, getInstrProfDataVarPrefix(), Renamed);
881+
std::string CntsVarName = getVarName(Inc, getInstrProfCountersVarPrefix());
882+
std::string DataVarName = getVarName(Inc, getInstrProfDataVarPrefix());
890883
auto MaybeSetComdat = [&](GlobalVariable *GV) {
891884
bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
892885
if (UseComdat) {
@@ -927,7 +920,7 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
927920
ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS);
928921
auto *ValuesVar = new GlobalVariable(
929922
*M, ValuesTy, false, Linkage, Constant::getNullValue(ValuesTy),
930-
getVarName(Inc, getInstrProfValuesVarPrefix(), Renamed));
923+
getVarName(Inc, getInstrProfValuesVarPrefix()));
931924
ValuesVar->setVisibility(Visibility);
932925
ValuesVar->setSection(
933926
getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
@@ -962,13 +955,8 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
962955
//
963956
// On COFF, a comdat leader cannot be local so we require DataReferencedByCode
964957
// to be false.
965-
//
966-
// If profd is in a deduplicate comdat, NS==0 with a hash suffix guarantees
967-
// that other copies must have the same CFG and cannot have value profiling.
968-
// If no hash suffix, other profd copies may be referenced by code.
969-
if (NS == 0 && !(NeedComdat && !Renamed) &&
970-
(TT.isOSBinFormatELF() ||
971-
(!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
958+
if (NS == 0 && (TT.isOSBinFormatELF() ||
959+
(!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
972960
Linkage = GlobalValue::PrivateLinkage;
973961
Visibility = GlobalValue::DefaultVisibility;
974962
}

llvm/test/Instrumentation/InstrProfiling/linkage.ll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,11 @@ define linkonce_odr void @foo_inline() {
7070
}
7171

7272
; ELF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_cnts", comdat, align 8
73-
; ELF: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
73+
; ELF: @__profd_foo_extern = private global {{.*}}section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
7474
; MACHO: @__profc_foo_extern = linkonce_odr hidden global
7575
; MACHO: @__profd_foo_extern = linkonce_odr hidden global
7676
; COFF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfc$M", comdat, align 8
77-
; COFF: @__profd_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
77+
; COFF: @__profd_foo_extern = private global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
7878
define available_externally void @foo_extern() {
7979
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)
8080
ret void

llvm/test/Transforms/PGOProfile/comdat.ll

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,9 @@ define linkonce_odr void @linkonceodr() comdat {
1616
ret void
1717
}
1818

19-
;; weakodr in a comdat is not renamed. There is no guarantee that definitions in
20-
;; other modules don't have value profiling. profd should be conservatively
21-
;; non-private to prevent a caller from referencing a non-prevailing profd,
22-
;; causing a linker error.
19+
;; weakodr in a comdat is not renamed.
2320
; ELF: @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
24-
; ELF: @__profd_weakodr = weak_odr hidden global {{.*}} comdat($__profc_weakodr), align 8
21+
; ELF: @__profd_weakodr = private global {{.*}} comdat($__profc_weakodr), align 8
2522
; COFF: @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
2623
; COFF: @__profd_weakodr = weak_odr hidden global {{.*}} comdat, align 8
2724
define weak_odr void @weakodr() comdat {

0 commit comments

Comments
 (0)