Skip to content

Commit f653bee

Browse files
committed
[InstrProfiling] Keep profd non-private for non-renamable comdat functions
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 pre-inlining PGO instrumentation (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. Reviewed By: xur Differential Revision: https://reviews.llvm.org/D108432
1 parent 98aa694 commit f653bee

File tree

3 files changed

+50
-9
lines changed

3 files changed

+50
-9
lines changed

llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp

+19-7
Original file line numberDiff line numberDiff line change
@@ -758,14 +758,18 @@ 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) {
761+
static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix,
762+
bool &Renamed) {
762763
StringRef NamePrefix = getInstrProfNameVarPrefix();
763764
StringRef Name = Inc->getName()->getName().substr(NamePrefix.size());
764765
Function *F = Inc->getParent()->getParent();
765766
Module *M = F->getParent();
766767
if (!DoHashBasedCounterSplit || !isIRPGOFlagSet(M) ||
767-
!canRenameComdatFunc(*F))
768+
!canRenameComdatFunc(*F)) {
769+
Renamed = false;
768770
return (Prefix + Name).str();
771+
}
772+
Renamed = true;
769773
uint64_t FuncHash = Inc->getHash()->getZExtValue();
770774
SmallVector<char, 24> HashPostfix;
771775
if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix)))
@@ -878,8 +882,11 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
878882
// discarded.
879883
bool DataReferencedByCode = profDataReferencedByCode(*M);
880884
bool NeedComdat = needsComdatForCounter(*Fn, *M);
881-
std::string CntsVarName = getVarName(Inc, getInstrProfCountersVarPrefix());
882-
std::string DataVarName = getVarName(Inc, getInstrProfDataVarPrefix());
885+
bool Renamed;
886+
std::string CntsVarName =
887+
getVarName(Inc, getInstrProfCountersVarPrefix(), Renamed);
888+
std::string DataVarName =
889+
getVarName(Inc, getInstrProfDataVarPrefix(), Renamed);
883890
auto MaybeSetComdat = [&](GlobalVariable *GV) {
884891
bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
885892
if (UseComdat) {
@@ -920,7 +927,7 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
920927
ArrayType *ValuesTy = ArrayType::get(Type::getInt64Ty(Ctx), NS);
921928
auto *ValuesVar = new GlobalVariable(
922929
*M, ValuesTy, false, Linkage, Constant::getNullValue(ValuesTy),
923-
getVarName(Inc, getInstrProfValuesVarPrefix()));
930+
getVarName(Inc, getInstrProfValuesVarPrefix(), Renamed));
924931
ValuesVar->setVisibility(Visibility);
925932
ValuesVar->setSection(
926933
getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
@@ -955,8 +962,13 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
955962
//
956963
// On COFF, a comdat leader cannot be local so we require DataReferencedByCode
957964
// to be false.
958-
if (NS == 0 && (TT.isOSBinFormatELF() ||
959-
(!DataReferencedByCode && TT.isOSBinFormatCOFF()))) {
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()))) {
960972
Linkage = GlobalValue::PrivateLinkage;
961973
Visibility = GlobalValue::DefaultVisibility;
962974
}

llvm/test/Instrumentation/InstrProfiling/linkage.ll

+2-2
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 = private global {{.*}}section "__llvm_prf_data", comdat($__profc_foo_extern), align 8
73+
; ELF: @__profd_foo_extern = linkonce_odr hidden 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 = private global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
77+
; COFF: @__profd_foo_extern = linkonce_odr hidden 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
+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
;; Test comdat functions.
2+
; RUN: opt < %s -mtriple=x86_64-linux -passes=pgo-instr-gen,instrprof -S | FileCheck %s --check-prefixes=ELF
3+
; RUN: opt < %s -mtriple=x86_64-windows -passes=pgo-instr-gen,instrprof -S | FileCheck %s --check-prefixes=COFF
4+
5+
$linkonceodr = comdat any
6+
$weakodr = comdat any
7+
8+
;; profc/profd have hash suffixes. This definition doesn't have value profiling,
9+
;; so definitions with the same name in other modules must have the same CFG and
10+
;; cannot have value profiling, either. profd can be made private for ELF.
11+
; ELF: @__profc_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
12+
; ELF: @__profd_linkonceodr.[[#]] = private global {{.*}} comdat($__profc_linkonceodr.[[#]]), align 8
13+
; COFF: @__profc_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
14+
; COFF: @__profd_linkonceodr.[[#]] = linkonce_odr hidden global {{.*}} comdat, align 8
15+
define linkonce_odr void @linkonceodr() comdat {
16+
ret void
17+
}
18+
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.
23+
; ELF: @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
24+
; ELF: @__profd_weakodr = weak_odr hidden global {{.*}} comdat($__profc_weakodr), align 8
25+
; COFF: @__profc_weakodr = weak_odr hidden global {{.*}} comdat, align 8
26+
; COFF: @__profd_weakodr = weak_odr hidden global {{.*}} comdat, align 8
27+
define weak_odr void @weakodr() comdat {
28+
ret void
29+
}

0 commit comments

Comments
 (0)