[llvm-branch-commits] [llvm] [PGO] Generate __llvm_profile_raw_version only when instrumented (PR #93917)
https://github.com/samolisov created
https://github.com/llvm/llvm-project/pull/93917
Currently, only modules that contain at least a single function
definition are instrumented. When a module contains no function
definitions at all, the module is not instrumented (yet?) and there is
no reason to introduce the '__llvm_profile_raw_version' variable into
the module.
>From ac467402f0d688c8295bbca0f03161516b6982df Mon Sep 17 00:00:00 2001
From: Pavel Samolysov
Date: Fri, 31 May 2024 05:46:25 +0300
Subject: [PATCH 1/2] [PGO] Preserve analysis results when nothing was
instrumented
The 'PGOInstrumentationGen' pass should preserve all analysis results
when nothing was actually instrumented. Currently, only modules that
contain at least a single function definition are instrumented. When a
module contains only function declarations and, optionally, global
variable definitions (a module for the regular-LTO phase for thin-LTO
when LTOUnit splitting is enabled, for example), such module is not
instrumented (yet?) and there is no reason to invalidate any analysis
results.
NFC.
---
.../Instrumentation/PGOInstrumentation.cpp| 5 ++
.../PGOInstrumentationTest.cpp| 66 ++-
2 files changed, 54 insertions(+), 17 deletions(-)
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index 2269c2e0fffae..fbf9688ed2d1e 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1845,6 +1845,7 @@ static bool InstrumentAllFunctions(
std::unordered_multimap ComdatMembers;
collectComdatMembers(M, ComdatMembers);
+ bool AnythingInstrumented = false;
for (auto &F : M) {
if (skipPGOGen(F))
continue;
@@ -1852,7 +1853,11 @@ static bool InstrumentAllFunctions(
auto *BPI = LookupBPI(F);
auto *BFI = LookupBFI(F);
instrumentOneFunc(F, &M, TLI, BPI, BFI, ComdatMembers, IsCS);
+AnythingInstrumented = true;
}
+ if (!AnythingInstrumented)
+return false;
+
return true;
}
diff --git
a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
index 02c2df2a138b0..cbeaa501d4d88 100644
--- a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
+++ b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
@@ -104,9 +104,13 @@ class MockModuleAnalysisHandle
ModuleAnalysisManager::Invalidator &));
};
-struct PGOInstrumentationGenTest
-: public Test,
- WithParamInterface> {
+template struct PGOTestName {
+ std::string operator()(const TestParamInfo &Info) const {
+return std::get<1>(Info.param).str();
+ }
+};
+
+struct PGOInstrumentationGenTest : public Test {
LLVMContext Ctx;
ModulePassManager MPM;
PassBuilder PB;
@@ -143,12 +147,47 @@ struct PGOInstrumentationGenTest
}
};
+struct PGOInstrumentationGenInstrumentTest
+: PGOInstrumentationGenTest,
+ WithParamInterface> {};
+
static constexpr StringRef CodeWithFuncDefs = R"(
define i32 @f(i32 %n) {
entry:
ret i32 0
})";
+INSTANTIATE_TEST_SUITE_P(
+PGOInstrumetationGenTestSuite, PGOInstrumentationGenInstrumentTest,
+Values(std::make_tuple(CodeWithFuncDefs, "instrument_function_defs")),
+PGOTestName());
+
+TEST_P(PGOInstrumentationGenInstrumentTest, Instrumented) {
+ const StringRef Code = std::get<0>(GetParam());
+ parseAssembly(Code);
+
+ ASSERT_THAT(M, NotNull());
+
+ Sequence PassSequence;
+ EXPECT_CALL(MMAHandle, run(Ref(*M), _))
+ .InSequence(PassSequence)
+ .WillOnce(DoDefault());
+ EXPECT_CALL(MMAHandle, invalidate(Ref(*M), _, _))
+ .InSequence(PassSequence)
+ .WillOnce(DoDefault());
+
+ MPM.run(*M, MAM);
+
+ const auto *IRInstrVar =
+ M->getNamedGlobal(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
+ EXPECT_THAT(IRInstrVar, NotNull());
+ EXPECT_FALSE(IRInstrVar->isDeclaration());
+}
+
+struct PGOInstrumentationGenIgnoreTest
+: PGOInstrumentationGenTest,
+ WithParamInterface> {};
+
static constexpr StringRef CodeWithFuncDecls = R"(
declare i32 @f(i32);
)";
@@ -159,27 +198,20 @@ static constexpr StringRef CodeWithGlobals = R"(
)";
INSTANTIATE_TEST_SUITE_P(
-PGOInstrumetationGenTestSuite, PGOInstrumentationGenTest,
-Values(std::make_tuple(CodeWithFuncDefs, "instrument_function_defs"),
- std::make_tuple(CodeWithFuncDecls, "instrument_function_decls"),
+PGOInstrumetationGenIgnoreTestSuite, PGOInstrumentationGenIgnoreTest,
+Values(std::make_tuple(CodeWithFuncDecls, "instrument_function_decls"),
std::make_tuple(CodeWithGlobals, "instrument_globals")),
-[](const TestParamInfo &Info) {
- return std::get<1>(Info.param).str();
-});
+PGOTestName());
-TEST_P(PGOInstrumentationGenTest, Instrumented) {
+TEST_P(PGOInstrumentationGenIgnoreTest, NotInstrum
[llvm-branch-commits] [llvm] [PGO] Generate __llvm_profile_raw_version only when instrumented (PR #93917)
samolisov wrote: It seems, github doesn't allow to modify the source branch for a PR, so I cannot convert an already opened PR https://github.com/llvm/llvm-project/pull/93421 into a stacked one. I've pushed exactly the same commits into the https://github.com/llvm/llvm-project/tree/users/psamolysov/pgo-instrument-when-at-least-one-definition branch and opened this PR into that branch. There are some conflicts but I believe they will be solved after landing the changes from https://github.com/llvm/llvm-project/pull/93421 into main. This commit is reviewable: https://github.com/llvm/llvm-project/commit/5d5ead1dbd9aac486aada3acf81cc09464ab7bae https://github.com/llvm/llvm-project/pull/93917 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [PGO] Generate __llvm_profile_raw_version only when instrumented (PR #93917)
samolisov wrote: @aeubanks my intent is to get no `__llvm_profile_raw_version` in modules where nothing was actually instrumented. Usually, in my cases, they are the modules for regular-LTO part in thin-LTO where split LTO Units are enabled. This is a good point about consistency and using the variable as a flag that a module had already been instrumented but when a split LTO unit contains the variable in both parts it makes problems for the linker: I know how to distinguish one module from another and change the linkage for the variable but I have no idea how to deal with two parts of the same module. May be this solution looks as a hack but I think it makes sense: nothing was instrumented and nothing makes a reason for the version variable to present. But I'm unfamiliar about details of the profile runtime library, if the absence of the version variable makes troubles it is a reason to preserve it. But actually I tried to link such modules, without the variable, and get some profile. https://github.com/llvm/llvm-project/pull/93917 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] [NFC][BOLT] Rename createDummyReturnFunction to createReturnBody (PR #98448)
https://github.com/samolisov approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/98448 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] fe67401 - [NFC][ScheduleDAG] Use structure bindings and emplace_back
Author: Pavel Samolysov
Date: 2022-09-12T15:38:11+03:00
New Revision: fe67401fd8140787b0591bb0e1de39ad78f34456
URL:
https://github.com/llvm/llvm-project/commit/fe67401fd8140787b0591bb0e1de39ad78f34456
DIFF:
https://github.com/llvm/llvm-project/commit/fe67401fd8140787b0591bb0e1de39ad78f34456.diff
LOG: [NFC][ScheduleDAG] Use structure bindings and emplace_back
Some uses of std::make_pair and the std::pair's first/second members
in the ScheduleDAGRRList.cpp file were replaced with using of the
vector's emplace_back along with structure bindings from C++17.
Added:
Modified:
llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
Removed:
diff --git a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
index 12450b6602ea..04a8b4d3b42e 100644
--- a/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
@@ -1204,11 +1204,11 @@ SUnit *ScheduleDAGRRList::CopyAndMoveSuccessors(SUnit
*SU) {
D.setSUnit(NewSU);
AddPredQueued(SuccSU, D);
D.setSUnit(SU);
- DelDeps.push_back(std::make_pair(SuccSU, D));
+ DelDeps.emplace_back(SuccSU, D);
}
}
- for (auto &DelDep : DelDeps)
-RemovePred(DelDep.first, DelDep.second);
+ for (const auto &[DelSU, DelD] : DelDeps)
+RemovePred(DelSU, DelD);
AvailableQueue->updateNode(SU);
AvailableQueue->addNode(NewSU);
@@ -1242,17 +1242,17 @@ void ScheduleDAGRRList::InsertCopiesAndMoveSuccs(SUnit
*SU, unsigned Reg,
SDep D = Succ;
D.setSUnit(CopyToSU);
AddPredQueued(SuccSU, D);
- DelDeps.push_back(std::make_pair(SuccSU, Succ));
+ DelDeps.emplace_back(SuccSU, Succ);
}
else {
- // Avoid scheduling the def-side copy before other successors. Otherwise
+ // Avoid scheduling the def-side copy before other successors. Otherwise,
// we could introduce another physreg interference on the copy and
// continue inserting copies indefinitely.
AddPredQueued(SuccSU, SDep(CopyFromSU, SDep::Artificial));
}
}
- for (auto &DelDep : DelDeps)
-RemovePred(DelDep.first, DelDep.second);
+ for (const auto &[DelSU, DelD] : DelDeps)
+RemovePred(DelSU, DelD);
SDep FromDep(SU, SDep::Data, Reg);
FromDep.setLatency(SU->Latency);
@@ -1484,16 +1484,15 @@ SUnit *ScheduleDAGRRList::PickNodeToScheduleBottomUp() {
if (LRegs[0] == TRI->getNumRegs()) dbgs() << "CallResource";
else dbgs() << printReg(LRegs[0], TRI);
dbgs() << " SU #" << CurSU->NodeNum << '\n');
- std::pair LRegsPair =
-LRegsMap.insert(std::make_pair(CurSU, LRegs));
- if (LRegsPair.second) {
+ auto [LRegsIter, LRegsInserted] = LRegsMap.try_emplace(CurSU, LRegs);
+ if (LRegsInserted) {
CurSU->isPending = true; // This SU is not in AvailableQueue right
now.
Interferences.push_back(CurSU);
}
else {
assert(CurSU->isPending && "Interferences are pending");
// Update the interference with current live regs.
-LRegsPair.first->second = LRegs;
+LRegsIter->second = LRegs;
}
CurSU = AvailableQueue->pop();
}
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [llvm] 02aaf8e - [NFC][ScheduleDAGInstrs] Use structure bindings and emplace_back
Author: Pavel Samolysov
Date: 2022-09-13T12:49:04+03:00
New Revision: 02aaf8e3d6e73116648afcbb691839ecec80aa0e
URL:
https://github.com/llvm/llvm-project/commit/02aaf8e3d6e73116648afcbb691839ecec80aa0e
DIFF:
https://github.com/llvm/llvm-project/commit/02aaf8e3d6e73116648afcbb691839ecec80aa0e.diff
LOG: [NFC][ScheduleDAGInstrs] Use structure bindings and emplace_back
Some uses of std::make_pair and the std::pair's first/second members
in the ScheduleDAGInstrs.[cpp|h] files were replaced with using of the
vector's emplace_back along with structure bindings from C++17.
Added:
Modified:
llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h
llvm/lib/CodeGen/ScheduleDAGInstrs.cpp
Removed:
diff --git a/llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h
b/llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h
index fb3900b4a9c14..dc8f02e28adf4 100644
--- a/llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h
+++ b/llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
diff --git a/llvm/lib/CodeGen/ScheduleDAGInstrs.cpp
b/llvm/lib/CodeGen/ScheduleDAGInstrs.cpp
index 4fc9399c2b9e9..abf04888abdbd 100644
--- a/llvm/lib/CodeGen/ScheduleDAGInstrs.cpp
+++ b/llvm/lib/CodeGen/ScheduleDAGInstrs.cpp
@@ -12,6 +12,7 @@
//===--===//
#include "llvm/CodeGen/ScheduleDAGInstrs.h"
+
#include "llvm/ADT/IntEqClasses.h"
#include "llvm/ADT/MapVector.h"
#include "llvm/ADT/SmallVector.h"
@@ -53,7 +54,6 @@
#include
#include
#include
-#include
#include
#include
@@ -92,12 +92,12 @@ static unsigned getReductionSize() {
return ReductionSize;
}
-static void dumpSUList(ScheduleDAGInstrs::SUList &L) {
+static void dumpSUList(const ScheduleDAGInstrs::SUList &L) {
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
dbgs() << "{ ";
- for (const SUnit *su : L) {
-dbgs() << "SU(" << su->NodeNum << ")";
-if (su != L.back())
+ for (const SUnit *SU : L) {
+dbgs() << "SU(" << SU->NodeNum << ")";
+if (SU != L.back())
dbgs() << ", ";
}
dbgs() << "}\n";
@@ -125,7 +125,7 @@ static bool getUnderlyingObjectsForInstr(const MachineInstr
*MI,
const MachineFrameInfo &MFI,
UnderlyingObjectsVector &Objects,
const DataLayout &DL) {
- auto allMMOsOkay = [&]() {
+ auto AllMMOsOkay = [&]() {
for (const MachineMemOperand *MMO : MI->memoperands()) {
// TODO: Figure out whether isAtomic is really necessary (see D57601).
if (MMO->isVolatile() || MMO->isAtomic())
@@ -147,7 +147,7 @@ static bool getUnderlyingObjectsForInstr(const MachineInstr
*MI,
return false;
bool MayAlias = PSV->mayAlias(&MFI);
-Objects.push_back(UnderlyingObjectsVector::value_type(PSV, MayAlias));
+Objects.emplace_back(PSV, MayAlias);
} else if (const Value *V = MMO->getValue()) {
SmallVector Objs;
if (!getUnderlyingObjectsForCodeGen(V, Objs))
@@ -155,7 +155,7 @@ static bool getUnderlyingObjectsForInstr(const MachineInstr
*MI,
for (Value *V : Objs) {
assert(isIdentifiedObject(V));
- Objects.push_back(UnderlyingObjectsVector::value_type(V, true));
+ Objects.emplace_back(V, true);
}
} else
return false;
@@ -163,7 +163,7 @@ static bool getUnderlyingObjectsForInstr(const MachineInstr
*MI,
return true;
};
- if (!allMMOsOkay()) {
+ if (!AllMMOsOkay()) {
Objects.clear();
return false;
}
@@ -676,9 +676,9 @@ void ScheduleDAGInstrs::addChainDependencies(SUnit *SU,
void ScheduleDAGInstrs::addBarrierChain(Value2SUsMap &map) {
assert(BarrierChain != nullptr);
- for (auto &I : map) {
-SUList &sus = I.second;
-for (auto *SU : sus)
+ for (auto &[V, SUs] : map) {
+(void)V;
+for (auto *SU : SUs)
SU->addPredBarrier(BarrierChain);
}
map.clear();
@@ -793,7 +793,7 @@ void ScheduleDAGInstrs::buildSchedGraph(AAResults *AA,
MII != MIE; --MII) {
MachineInstr &MI = *std::prev(MII);
if (DbgMI) {
- DbgValues.push_back(std::make_pair(DbgMI, &MI));
+ DbgValues.emplace_back(DbgMI, &MI);
DbgMI = nullptr;
}
@@ -1019,21 +1019,21 @@ raw_ostream &llvm::operator<<(raw_ostream &OS, const
PseudoSourceValue* PSV) {
}
void ScheduleDAGInstrs::Value2SUsMap::dump() {
- for (auto &Itr : *this) {
-if (Itr.first.is()) {
- const Value *V = Itr.first.get();
+ for (const auto &[ValType, SUs] : *this) {
+if (ValType.is()) {
+ const Value *V = ValType.get();
if (isa(V))
dbgs() << "Unknown";
else
V->printAsOperand(dbgs());
}
-else if (Itr.first.is())
- dbgs() << Itr.first.get();
+else if (ValType.is())
+ dbgs() << Val
