[llvm] [PGO] Preserve analysis results when nothing was instrumented (PR #93421)
Pavel Samolysov via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 14 19:13:43 PDT 2024
https://github.com/samolisov updated https://github.com/llvm/llvm-project/pull/93421
>From 37019cf6165d1d947b06995e4937e958de72d3b2 Mon Sep 17 00:00:00 2001
From: Pavel Samolysov <samolisov at gmail.com>
Date: Wed, 22 May 2024 22:10:55 +0300
Subject: [PATCH 1/3] [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 introduce the
'__llvm_profile_raw_version' variable into the module.
---
.../Instrumentation/PGOInstrumentation.cpp | 25 +++++--
.../PGOProfile/declarations_only.ll | 7 +-
.../PGOProfile/global_variables_only.ll | 5 +-
.../PGOProfile/no_global_variables.ll | 15 ++++
.../PGOInstrumentationTest.cpp | 69 ++++++++++++++-----
5 files changed, 85 insertions(+), 36 deletions(-)
create mode 100644 llvm/test/Transforms/PGOProfile/no_global_variables.ll
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index 2269c2e0fffae..ff94fc98d0117 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -405,9 +405,14 @@ static GlobalVariable *createIRLevelProfileFlagVar(Module &M, bool IsCS) {
ProfileVersion |= VARIANT_MASK_BYTE_COVERAGE;
if (PGOTemporalInstrumentation)
ProfileVersion |= VARIANT_MASK_TEMPORAL_PROF;
- auto IRLevelVersionVariable = new GlobalVariable(
+ assert(!M.global_empty() &&
+ "If a module was instrumented, there must be defined global variables "
+ "at least for the counters.");
+ auto *InsertionPoint = &*M.global_begin();
+ auto *IRLevelVersionVariable = new GlobalVariable(
M, IntTy64, true, GlobalValue::WeakAnyLinkage,
- Constant::getIntegerValue(IntTy64, APInt(64, ProfileVersion)), VarName);
+ Constant::getIntegerValue(IntTy64, APInt(64, ProfileVersion)), VarName,
+ InsertionPoint);
IRLevelVersionVariable->setVisibility(GlobalValue::HiddenVisibility);
Triple TT(M.getTargetTriple());
if (TT.supportsCOMDAT()) {
@@ -1829,11 +1834,6 @@ static bool InstrumentAllFunctions(
Module &M, function_ref<TargetLibraryInfo &(Function &)> LookupTLI,
function_ref<BranchProbabilityInfo *(Function &)> LookupBPI,
function_ref<BlockFrequencyInfo *(Function &)> LookupBFI, bool IsCS) {
- // For the context-sensitve instrumentation, we should have a separated pass
- // (before LTO/ThinLTO linking) to create these variables.
- if (!IsCS && !PGOCtxProfLoweringPass::isContextualIRPGOEnabled())
- createIRLevelProfileFlagVar(M, /*IsCS=*/false);
-
Triple TT(M.getTargetTriple());
LLVMContext &Ctx = M.getContext();
if (!TT.isOSBinFormatELF() && EnableVTableValueProfiling)
@@ -1845,6 +1845,7 @@ static bool InstrumentAllFunctions(
std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
collectComdatMembers(M, ComdatMembers);
+ bool AnythingInstrumented = false;
for (auto &F : M) {
if (skipPGOGen(F))
continue;
@@ -1852,7 +1853,17 @@ 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;
+
+ // For the context-sensitve instrumentation, we should have a separated pass
+ // (before LTO/ThinLTO linking) to create these variables.
+ if (!IsCS && !PGOCtxProfLoweringPass::isContextualIRPGOEnabled())
+ createIRLevelProfileFlagVar(M, /*IsCS=*/false);
+
return true;
}
diff --git a/llvm/test/Transforms/PGOProfile/declarations_only.ll b/llvm/test/Transforms/PGOProfile/declarations_only.ll
index e7208fc264c7c..7a975725410c9 100644
--- a/llvm/test/Transforms/PGOProfile/declarations_only.ll
+++ b/llvm/test/Transforms/PGOProfile/declarations_only.ll
@@ -1,13 +1,8 @@
-; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN --check-prefix=GEN-COMDAT
+; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --implicit-check-not='__llvm_profile_raw_version'
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
-; GEN-COMDAT: $__llvm_profile_raw_version = comdat any
-; GEN-COMDAT: @__llvm_profile_raw_version = hidden constant i64 {{[0-9]+}}, comdat
-; GEN-NOT: @__profn_test_1 = private constant [6 x i8] c"test_1"
-; GEN-NOT: @__profn_test_2 = private constant [6 x i8] c"test_2"
-
declare i32 @test_1(i32 %i)
declare i32 @test_2(i32 %i)
diff --git a/llvm/test/Transforms/PGOProfile/global_variables_only.ll b/llvm/test/Transforms/PGOProfile/global_variables_only.ll
index 3bfa29af5d34f..46628bfafe534 100644
--- a/llvm/test/Transforms/PGOProfile/global_variables_only.ll
+++ b/llvm/test/Transforms/PGOProfile/global_variables_only.ll
@@ -1,9 +1,6 @@
-; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN-COMDAT
+; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --implicit-check-not='__llvm_profile_raw_version'
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
-; GEN-COMDAT: $__llvm_profile_raw_version = comdat any
-; GEN-COMDAT: @__llvm_profile_raw_version = hidden constant i64 {{[0-9]+}}, comdat
-
@var = internal unnamed_addr global [35 x ptr] zeroinitializer, align 16
diff --git a/llvm/test/Transforms/PGOProfile/no_global_variables.ll b/llvm/test/Transforms/PGOProfile/no_global_variables.ll
new file mode 100644
index 0000000000000..0faa2f35274c9
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/no_global_variables.ll
@@ -0,0 +1,15 @@
+; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN-COMDAT
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+; GEN-COMDAT: $__llvm_profile_raw_version = comdat any
+; GEN-COMDAT: @__llvm_profile_raw_version = hidden constant i64 {{[0-9]+}}, comdat
+
+define i32 @test_1(i32 %i) {
+entry:
+ ret i32 0
+}
+
+define i32 @test_2(i32 %i) {
+entry:
+ ret i32 1
+}
diff --git a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
index 02c2df2a138b0..01d17c5059a1a 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<std::tuple<StringRef, StringRef>> {
+template <typename ParamType> struct PGOTestName {
+ std::string operator()(const TestParamInfo<ParamType> &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<std::tuple<StringRef, StringRef>> {};
+
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<PGOInstrumentationGenInstrumentTest::ParamType>());
+
+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<std::tuple<StringRef, StringRef>> {};
+
static constexpr StringRef CodeWithFuncDecls = R"(
declare i32 @f(i32);
)";
@@ -159,34 +198,26 @@ 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<PGOInstrumentationGenTest::ParamType> &Info) {
- return std::get<1>(Info.param).str();
- });
+ PGOTestName<PGOInstrumentationGenIgnoreTest::ParamType>());
-TEST_P(PGOInstrumentationGenTest, Instrumented) {
+TEST_P(PGOInstrumentationGenIgnoreTest, NotInstrumented) {
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());
+ EXPECT_CALL(MMAHandle, run(Ref(*M), _)).WillOnce(DoDefault());
+ EXPECT_CALL(MMAHandle, invalidate(Ref(*M), _, _)).Times(0);
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());
+ EXPECT_THAT(IRInstrVar, IsNull());
}
} // end anonymous namespace
>From e6a918ea937465483ad942001a220ee6742aea4f Mon Sep 17 00:00:00 2001
From: Pavel Samolysov <samolisov at gmail.com>
Date: Fri, 31 May 2024 05:57:41 +0300
Subject: [PATCH 2/3] Revert "[PGO] Generate __llvm_profile_raw_version only
when instrumented"
This reverts commit 5d5ead1dbd9aac486aada3acf81cc09464ab7bae.
---
.../Instrumentation/PGOInstrumentation.cpp | 20 +++++++------------
.../PGOProfile/declarations_only.ll | 7 ++++++-
.../PGOProfile/global_variables_only.ll | 5 ++++-
.../PGOProfile/no_global_variables.ll | 15 --------------
.../PGOInstrumentationTest.cpp | 3 ++-
5 files changed, 19 insertions(+), 31 deletions(-)
delete mode 100644 llvm/test/Transforms/PGOProfile/no_global_variables.ll
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index ff94fc98d0117..fbf9688ed2d1e 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -405,14 +405,9 @@ static GlobalVariable *createIRLevelProfileFlagVar(Module &M, bool IsCS) {
ProfileVersion |= VARIANT_MASK_BYTE_COVERAGE;
if (PGOTemporalInstrumentation)
ProfileVersion |= VARIANT_MASK_TEMPORAL_PROF;
- assert(!M.global_empty() &&
- "If a module was instrumented, there must be defined global variables "
- "at least for the counters.");
- auto *InsertionPoint = &*M.global_begin();
- auto *IRLevelVersionVariable = new GlobalVariable(
+ auto IRLevelVersionVariable = new GlobalVariable(
M, IntTy64, true, GlobalValue::WeakAnyLinkage,
- Constant::getIntegerValue(IntTy64, APInt(64, ProfileVersion)), VarName,
- InsertionPoint);
+ Constant::getIntegerValue(IntTy64, APInt(64, ProfileVersion)), VarName);
IRLevelVersionVariable->setVisibility(GlobalValue::HiddenVisibility);
Triple TT(M.getTargetTriple());
if (TT.supportsCOMDAT()) {
@@ -1834,6 +1829,11 @@ static bool InstrumentAllFunctions(
Module &M, function_ref<TargetLibraryInfo &(Function &)> LookupTLI,
function_ref<BranchProbabilityInfo *(Function &)> LookupBPI,
function_ref<BlockFrequencyInfo *(Function &)> LookupBFI, bool IsCS) {
+ // For the context-sensitve instrumentation, we should have a separated pass
+ // (before LTO/ThinLTO linking) to create these variables.
+ if (!IsCS && !PGOCtxProfLoweringPass::isContextualIRPGOEnabled())
+ createIRLevelProfileFlagVar(M, /*IsCS=*/false);
+
Triple TT(M.getTargetTriple());
LLVMContext &Ctx = M.getContext();
if (!TT.isOSBinFormatELF() && EnableVTableValueProfiling)
@@ -1855,15 +1855,9 @@ static bool InstrumentAllFunctions(
instrumentOneFunc(F, &M, TLI, BPI, BFI, ComdatMembers, IsCS);
AnythingInstrumented = true;
}
-
if (!AnythingInstrumented)
return false;
- // For the context-sensitve instrumentation, we should have a separated pass
- // (before LTO/ThinLTO linking) to create these variables.
- if (!IsCS && !PGOCtxProfLoweringPass::isContextualIRPGOEnabled())
- createIRLevelProfileFlagVar(M, /*IsCS=*/false);
-
return true;
}
diff --git a/llvm/test/Transforms/PGOProfile/declarations_only.ll b/llvm/test/Transforms/PGOProfile/declarations_only.ll
index 7a975725410c9..e7208fc264c7c 100644
--- a/llvm/test/Transforms/PGOProfile/declarations_only.ll
+++ b/llvm/test/Transforms/PGOProfile/declarations_only.ll
@@ -1,8 +1,13 @@
-; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --implicit-check-not='__llvm_profile_raw_version'
+; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN --check-prefix=GEN-COMDAT
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
+; GEN-COMDAT: $__llvm_profile_raw_version = comdat any
+; GEN-COMDAT: @__llvm_profile_raw_version = hidden constant i64 {{[0-9]+}}, comdat
+; GEN-NOT: @__profn_test_1 = private constant [6 x i8] c"test_1"
+; GEN-NOT: @__profn_test_2 = private constant [6 x i8] c"test_2"
+
declare i32 @test_1(i32 %i)
declare i32 @test_2(i32 %i)
diff --git a/llvm/test/Transforms/PGOProfile/global_variables_only.ll b/llvm/test/Transforms/PGOProfile/global_variables_only.ll
index 46628bfafe534..3bfa29af5d34f 100644
--- a/llvm/test/Transforms/PGOProfile/global_variables_only.ll
+++ b/llvm/test/Transforms/PGOProfile/global_variables_only.ll
@@ -1,6 +1,9 @@
-; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --implicit-check-not='__llvm_profile_raw_version'
+; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN-COMDAT
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
+; GEN-COMDAT: $__llvm_profile_raw_version = comdat any
+; GEN-COMDAT: @__llvm_profile_raw_version = hidden constant i64 {{[0-9]+}}, comdat
+
@var = internal unnamed_addr global [35 x ptr] zeroinitializer, align 16
diff --git a/llvm/test/Transforms/PGOProfile/no_global_variables.ll b/llvm/test/Transforms/PGOProfile/no_global_variables.ll
deleted file mode 100644
index 0faa2f35274c9..0000000000000
--- a/llvm/test/Transforms/PGOProfile/no_global_variables.ll
+++ /dev/null
@@ -1,15 +0,0 @@
-; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN-COMDAT
-target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-unknown-linux-gnu"
-; GEN-COMDAT: $__llvm_profile_raw_version = comdat any
-; GEN-COMDAT: @__llvm_profile_raw_version = hidden constant i64 {{[0-9]+}}, comdat
-
-define i32 @test_1(i32 %i) {
-entry:
- ret i32 0
-}
-
-define i32 @test_2(i32 %i) {
-entry:
- ret i32 1
-}
diff --git a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
index 01d17c5059a1a..cbeaa501d4d88 100644
--- a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
+++ b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
@@ -217,7 +217,8 @@ TEST_P(PGOInstrumentationGenIgnoreTest, NotInstrumented) {
const auto *IRInstrVar =
M->getNamedGlobal(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
- EXPECT_THAT(IRInstrVar, IsNull());
+ EXPECT_THAT(IRInstrVar, NotNull());
+ EXPECT_FALSE(IRInstrVar->isDeclaration());
}
} // end anonymous namespace
>From 944d5f59fa188e448c9ddd2d56f83aa135a75ba0 Mon Sep 17 00:00:00 2001
From: Pavel Samolysov <samolisov at gmail.com>
Date: Sat, 15 Jun 2024 05:12:43 +0300
Subject: [PATCH 3/3] Apply suggestions from @ellishg
---
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | 5 +----
.../Transforms/Instrumentation/PGOInstrumentationTest.cpp | 4 ++--
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index fbf9688ed2d1e..278245c6dd2b1 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1855,10 +1855,7 @@ static bool InstrumentAllFunctions(
instrumentOneFunc(F, &M, TLI, BPI, BFI, ComdatMembers, IsCS);
AnythingInstrumented = true;
}
- if (!AnythingInstrumented)
- return false;
-
- return true;
+ return AnythingInstrumented;
}
PreservedAnalyses
diff --git a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
index cbeaa501d4d88..8a8a06f42f9cb 100644
--- a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
+++ b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
@@ -180,7 +180,7 @@ TEST_P(PGOInstrumentationGenInstrumentTest, Instrumented) {
const auto *IRInstrVar =
M->getNamedGlobal(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
- EXPECT_THAT(IRInstrVar, NotNull());
+ ASSERT_THAT(IRInstrVar, NotNull());
EXPECT_FALSE(IRInstrVar->isDeclaration());
}
@@ -217,7 +217,7 @@ TEST_P(PGOInstrumentationGenIgnoreTest, NotInstrumented) {
const auto *IRInstrVar =
M->getNamedGlobal(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
- EXPECT_THAT(IRInstrVar, NotNull());
+ ASSERT_THAT(IRInstrVar, NotNull());
EXPECT_FALSE(IRInstrVar->isDeclaration());
}
More information about the llvm-commits
mailing list