[llvm] f653bee - [InstrProfiling] Keep profd non-private for non-renamable comdat functions
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 24 15:59:40 PDT 2021
Author: Fangrui Song
Date: 2021-08-24T15:59:35-07:00
New Revision: f653beea88d2684cdc8117e662b321ba04666771
URL: https://github.com/llvm/llvm-project/commit/f653beea88d2684cdc8117e662b321ba04666771
DIFF: https://github.com/llvm/llvm-project/commit/f653beea88d2684cdc8117e662b321ba04666771.diff
LOG: [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
Added:
llvm/test/Transforms/PGOProfile/comdat.ll
Modified:
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
llvm/test/Instrumentation/InstrProfiling/linkage.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index d2620fd593e15..aa383b71b40c7 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -758,14 +758,18 @@ void InstrProfiling::lowerCoverageData(GlobalVariable *CoverageNamesVar) {
}
/// 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)))
@@ -878,8 +882,11 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
// 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) {
@@ -920,7 +927,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()));
+ getVarName(Inc, getInstrProfValuesVarPrefix(), Renamed));
ValuesVar->setVisibility(Visibility);
ValuesVar->setSection(
getInstrProfSectionName(IPSK_vals, TT.getObjectFormat()));
@@ -955,8 +962,13 @@ InstrProfiling::getOrCreateRegionCounters(InstrProfIncrementInst *Inc) {
//
// 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() ||
+ (!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 bb381afeafbd2..bb7f33f9deb5d 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 = 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
diff --git a/llvm/test/Transforms/PGOProfile/comdat.ll b/llvm/test/Transforms/PGOProfile/comdat.ll
new file mode 100644
index 0000000000000..9f5c0ee848ca5
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/comdat.ll
@@ -0,0 +1,29 @@
+;; 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
+}
More information about the llvm-commits
mailing list