[llvm] [nfc][InstrProfiling]For comdat setting helper function, move comment closer to the code (PR #83757)
Mingming Liu via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 4 09:08:13 PST 2024
https://github.com/minglotus-6 updated https://github.com/llvm/llvm-project/pull/83757
>From 5b2116736df25782e85912b72146d0383035ef78 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Sun, 3 Mar 2024 18:49:06 -0800
Subject: [PATCH 1/3] [nfc][InstrProfiling]Compute a boolean state as a class
constant and use it everywhere
---
.../Instrumentation/InstrProfiling.cpp | 48 +++++++++----------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index dbd44bd36e1148..d5d55dec6382fb 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -168,13 +168,33 @@ cl::opt<bool> SkipRetExitBlock(
using LoadStorePair = std::pair<Instruction *, Instruction *>;
+static uint64_t getIntModuleFlagOrZero(const Module &M, StringRef Flag) {
+ auto *MD = dyn_cast_or_null<ConstantAsMetadata>(M.getModuleFlag(Flag));
+ if (!MD)
+ return 0;
+
+ // If the flag is a ConstantAsMetadata, it should be an integer representable
+ // in 64-bits.
+ return cast<ConstantInt>(MD->getValue())->getZExtValue();
+}
+
+static bool enablesValueProfiling(const Module &M) {
+ return isIRPGOFlagSet(&M) ||
+ getIntModuleFlagOrZero(M, "EnableValueProfiling") != 0;
+}
+
+// Conservatively returns true if value profiling is enabled.
+static bool profDataReferencedByCode(const Module &M) {
+ return enablesValueProfiling(M);
+}
+
class InstrLowerer final {
public:
InstrLowerer(Module &M, const InstrProfOptions &Options,
std::function<const TargetLibraryInfo &(Function &F)> GetTLI,
bool IsCS)
: M(M), Options(Options), TT(Triple(M.getTargetTriple())), IsCS(IsCS),
- GetTLI(GetTLI) {}
+ GetTLI(GetTLI), DataReferencedByCode(profDataReferencedByCode(M)) {}
bool lower();
@@ -186,6 +206,8 @@ class InstrLowerer final {
const bool IsCS;
std::function<const TargetLibraryInfo &(Function &F)> GetTLI;
+
+ const bool DataReferencedByCode;
struct PerFunctionProfileData {
uint32_t NumValueSites[IPVK_Last + 1] = {};
GlobalVariable *RegionCounters = nullptr;
@@ -1057,26 +1079,6 @@ static std::string getVarName(InstrProfInstBase *Inc, StringRef Prefix,
return (Prefix + Name + "." + Twine(FuncHash)).str();
}
-static uint64_t getIntModuleFlagOrZero(const Module &M, StringRef Flag) {
- auto *MD = dyn_cast_or_null<ConstantAsMetadata>(M.getModuleFlag(Flag));
- if (!MD)
- return 0;
-
- // If the flag is a ConstantAsMetadata, it should be an integer representable
- // in 64-bits.
- return cast<ConstantInt>(MD->getValue())->getZExtValue();
-}
-
-static bool enablesValueProfiling(const Module &M) {
- return isIRPGOFlagSet(&M) ||
- getIntModuleFlagOrZero(M, "EnableValueProfiling") != 0;
-}
-
-// Conservatively returns true if data variables may be referenced by code.
-static bool profDataReferencedByCode(const Module &M) {
- return enablesValueProfiling(M);
-}
-
static inline bool shouldRecordFunctionAddr(Function *F) {
// Only record function addresses if IR PGO is enabled or if clang value
// profiling is enabled. Recording function addresses greatly increases object
@@ -1192,7 +1194,6 @@ static bool needsRuntimeRegistrationOfSectionRange(const Triple &TT) {
void InstrLowerer::maybeSetComdat(GlobalVariable *GV, Function *Fn,
StringRef VarName) {
- bool DataReferencedByCode = profDataReferencedByCode(M);
bool NeedComdat = needsComdatForCounter(*Fn, M);
bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
@@ -1418,7 +1419,6 @@ void InstrLowerer::createDataVariable(InstrProfCntrInstBase *Inc) {
Visibility = GlobalValue::DefaultVisibility;
}
- bool DataReferencedByCode = profDataReferencedByCode(M);
bool NeedComdat = needsComdatForCounter(*Fn, M);
bool Renamed;
@@ -1719,7 +1719,7 @@ void InstrLowerer::emitUses() {
// and ensure this GC property as well. Otherwise, we have to conservatively
// make all of the sections retained by the linker.
if (TT.isOSBinFormatELF() || TT.isOSBinFormatMachO() ||
- (TT.isOSBinFormatCOFF() && !profDataReferencedByCode(M)))
+ (TT.isOSBinFormatCOFF() && !DataReferencedByCode))
appendToCompilerUsed(M, CompilerUsedVars);
else
appendToUsed(M, CompilerUsedVars);
>From 13099c90449036731b834e27aa8fb1c238ab8669 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Sun, 3 Mar 2024 19:02:09 -0800
Subject: [PATCH 2/3] [nfc][InstrProfiling]For comdat setting helper function,
move comment closer to the code
---
.../Instrumentation/InstrProfiling.cpp | 53 +++++++++++--------
1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index d5d55dec6382fb..84c6aef4c998a7 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -206,8 +206,8 @@ class InstrLowerer final {
const bool IsCS;
std::function<const TargetLibraryInfo &(Function &F)> GetTLI;
-
const bool DataReferencedByCode;
+
struct PerFunctionProfileData {
uint32_t NumValueSites[IPVK_Last + 1] = {};
GlobalVariable *RegionCounters = nullptr;
@@ -1193,18 +1193,42 @@ static bool needsRuntimeRegistrationOfSectionRange(const Triple &TT) {
}
void InstrLowerer::maybeSetComdat(GlobalVariable *GV, Function *Fn,
- StringRef VarName) {
+ StringRef CounterGroupName) {
+ // Place lowered global variables in a comdat group if the associated function
+ // is a COMDAT. This will make sure that only one copy of global variable
+ // (e.g. function counters) of the COMDAT function will be emitted after
+ // linking.
bool NeedComdat = needsComdatForCounter(*Fn, M);
+
bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
if (!UseComdat)
return;
- StringRef GroupName =
- TT.isOSBinFormatCOFF() && DataReferencedByCode ? GV->getName() : VarName;
+ // Keep in mind that this pass may run before the inliner, so we need to
+ // create a new comdat group (for counters, profiling data, etc). If we use
+ // the comdat of the parent function, that will result in relocations against
+ // discarded sections.
+ //
+ // If the data variable is referenced by code, non-counter variables (notably
+ // profiling data) and counters have to be in different comdats for COFF
+ // because the Visual C++ linker will report duplicate symbol errors if there
+ // are multiple external symbols with the same name marked
+ // IMAGE_COMDAT_SELECT_ASSOCIATIVE.
+ StringRef GroupName = TT.isOSBinFormatCOFF() && DataReferencedByCode
+ ? GV->getName()
+ : CounterGroupName;
Comdat *C = M.getOrInsertComdat(GroupName);
- if (!NeedComdat)
+
+ if (!NeedComdat) {
+ // Object file format must be ELF since `UseComdat && !NeedComdat` is true.
+ //
+ // 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.
C->setSelectionKind(Comdat::NoDeduplicate);
+ }
GV->setComdat(C);
// COFF doesn't allow the comdat group leader to have private linkage, so
// upgrade private linkage to internal linkage to produce a symbol table
@@ -1238,23 +1262,7 @@ GlobalVariable *InstrLowerer::setupProfileSection(InstrProfInstBase *Inc,
Linkage = GlobalValue::PrivateLinkage;
Visibility = GlobalValue::DefaultVisibility;
}
- // Move the name variable to the right section. Place them in a COMDAT group
- // if the associated function is a COMDAT. This will make sure that only one
- // copy of counters of the COMDAT function will be emitted after linking. Keep
- // in mind that this pass may run before the inliner, so we need to create a
- // new comdat group for the counters and profiling data. If we use the comdat
- // of the parent function, that will result in relocations against discarded
- // sections.
- //
- // If the data variable is referenced by code, counters and data have to be
- // in different comdats for COFF because the Visual C++ linker will report
- // duplicate symbol errors if there are multiple external symbols with the
- // 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.
+ // Move the name variable to the right section.
bool Renamed;
GlobalVariable *Ptr;
StringRef VarPrefix;
@@ -1524,6 +1532,7 @@ void InstrLowerer::createDataVariable(InstrProfCntrInstBase *Inc) {
Data->setSection(
getInstrProfSectionName(DataSectionKind, TT.getObjectFormat()));
Data->setAlignment(Align(INSTR_PROF_DATA_ALIGNMENT));
+
maybeSetComdat(Data, Fn, CntsVarName);
PD.DataVar = Data;
>From 28301273f25dac0219f591dd1ee18ef587ccec24 Mon Sep 17 00:00:00 2001
From: mingmingl <mingmingl at google.com>
Date: Sun, 3 Mar 2024 19:07:06 -0800
Subject: [PATCH 3/3] make blank lines consistent
---
llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index 84c6aef4c998a7..c42c53edd51190 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -206,6 +206,7 @@ class InstrLowerer final {
const bool IsCS;
std::function<const TargetLibraryInfo &(Function &F)> GetTLI;
+
const bool DataReferencedByCode;
struct PerFunctionProfileData {
@@ -1199,7 +1200,6 @@ void InstrLowerer::maybeSetComdat(GlobalVariable *GV, Function *Fn,
// (e.g. function counters) of the COMDAT function will be emitted after
// linking.
bool NeedComdat = needsComdatForCounter(*Fn, M);
-
bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
if (!UseComdat)
@@ -1532,7 +1532,6 @@ void InstrLowerer::createDataVariable(InstrProfCntrInstBase *Inc) {
Data->setSection(
getInstrProfSectionName(DataSectionKind, TT.getObjectFormat()));
Data->setAlignment(Align(INSTR_PROF_DATA_ALIGNMENT));
-
maybeSetComdat(Data, Fn, CntsVarName);
PD.DataVar = Data;
More information about the llvm-commits
mailing list