[PGO] Preserve analysis results when nothing was instrumented #93421
Conversation
samolisov
requested review from
mtrofin,
ellishg,
teresajohnson and
kazutakahirata
|
@llvm/pr-subscribers-pgo @llvm/pr-subscribers-llvm-transforms Author: Pavel Samolysov (samolisov) ChangesIt makes sense to instrument modules that contain at least a single function definition only. 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), it makes no sense to instrument such module and moreover, to introduce the The patch also introduces the gmock-based unittest infrastructure for PGO Instrumentation and adds some test cases to check whether the instrumentation has taken place. The testing infrastructure for analysis modules was borrowed from the Full diff: https://github.com/llvm/llvm-project/pull/93421.diff 7 Files Affected:
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
new file mode 100644
index 0000000000000..6d9c86cb8f7a7
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/declarations_only.ll
@@ -0,0 +1,7 @@
+; 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"
+
+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
new file mode 100644
index 0000000000000..c4b298a27bce5
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/global_variables_only.ll
@@ -0,0 +1,5 @@
+; 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"
+
+@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/CMakeLists.txt b/llvm/unittests/Transforms/CMakeLists.txt
index 98c821acde3a5..320cdf5674149 100644
--- a/llvm/unittests/Transforms/CMakeLists.txt
+++ b/llvm/unittests/Transforms/CMakeLists.txt
@@ -1,4 +1,5 @@
add_subdirectory(Coroutines)
+add_subdirectory(Instrumentation)
add_subdirectory(IPO)
add_subdirectory(Scalar)
add_subdirectory(Utils)
diff --git a/llvm/unittests/Transforms/Instrumentation/CMakeLists.txt b/llvm/unittests/Transforms/Instrumentation/CMakeLists.txt
new file mode 100644
index 0000000000000..1f249b0049d06
--- /dev/null
+++ b/llvm/unittests/Transforms/Instrumentation/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+ Analysis
+ AsmParser
+ Core
+ Instrumentation
+ Passes
+ Support
+)
+
+add_llvm_unittest(InstrumentationTests
+ PGOInstrumentationTest.cpp
+ )
+
+target_link_libraries(InstrumentationTests PRIVATE LLVMTestingSupport)
+
+set_property(TARGET InstrumentationTests PROPERTY FOLDER "Tests/UnitTests/TransformTests")
diff --git a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
new file mode 100644
index 0000000000000..670a869ea716b
--- /dev/null
+++ b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
@@ -0,0 +1,207 @@
+//===- PGOInstrumentationTest.cpp - Instrumentation unit tests ------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Transforms/Instrumentation/PGOInstrumentation.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Passes/PassBuilder.h"
+#include "llvm/ProfileData/InstrProf.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace {
+
+using namespace llvm;
+
+using ::testing::DoDefault;
+using ::testing::Invoke;
+using ::testing::NotNull;
+using ::testing::IsNull;
+using ::testing::Ref;
+using ::testing::Return;
+using ::testing::Sequence;
+using ::testing::Test;
+using testing::_;
+
+template <typename Derived> class MockAnalysisHandleBase {
+public:
+ class Analysis : public AnalysisInfoMixin<Analysis> {
+ public:
+ class Result {
+ public:
+ // Forward invalidation events to the mock handle.
+ bool invalidate(Module &M, const PreservedAnalyses &PA,
+ ModuleAnalysisManager::Invalidator &Inv) {
+ return Handle->invalidate(M, PA, Inv);
+ }
+ private:
+ explicit Result(Derived *Handle) : Handle(Handle) {}
+
+ friend MockAnalysisHandleBase;
+ Derived *Handle;
+ };
+
+ Result run(Module &M, ModuleAnalysisManager &AM) {
+ return Handle->run(M, AM);
+ }
+
+ private:
+ friend AnalysisInfoMixin<Analysis>;
+ friend MockAnalysisHandleBase;
+ static inline AnalysisKey Key;
+
+ Derived *Handle;
+
+ explicit Analysis(Derived *Handle) : Handle(Handle) {}
+ };
+
+ Analysis getAnalysis() {
+ return Analysis(static_cast<Derived *>(this));
+ }
+
+ typename Analysis::Result getResult() {
+ return typename Analysis::Result(static_cast<Derived *>(this));
+ }
+
+protected:
+ void setDefaults() {
+ ON_CALL(static_cast<Derived &>(*this), run(_, _))
+ .WillByDefault(Return(this->getResult()));
+ ON_CALL(static_cast<Derived &>(*this), invalidate(_, _, _))
+ .WillByDefault(Invoke([](Module &M, const PreservedAnalyses &PA,
+ ModuleAnalysisManager::Invalidator &) {
+ auto PAC = PA.template getChecker<Analysis>();
+ return !PAC.preserved() &&
+ !PAC.template preservedSet<AllAnalysesOn<Module>>();
+ }));
+ }
+
+private:
+ friend Derived;
+ MockAnalysisHandleBase() = default;
+};
+
+class MockModuleAnalysisHandle
+ : public MockAnalysisHandleBase<MockModuleAnalysisHandle> {
+public:
+ MockModuleAnalysisHandle() { setDefaults(); }
+
+ MOCK_METHOD(typename Analysis::Result, run,
+ (Module &, ModuleAnalysisManager &));
+
+ MOCK_METHOD(bool, invalidate,
+ (Module &, const PreservedAnalyses &,
+ ModuleAnalysisManager::Invalidator &));
+};
+
+struct PGOInstrumentationGenTest : public Test {
+ LLVMContext Ctx;
+ ModulePassManager MPM;
+ PassBuilder PB;
+ MockModuleAnalysisHandle MMAHandle;
+ LoopAnalysisManager LAM;
+ FunctionAnalysisManager FAM;
+ CGSCCAnalysisManager CGAM;
+ ModuleAnalysisManager MAM;
+ LLVMContext Context;
+ std::unique_ptr<Module> M;
+
+ PGOInstrumentationGenTest() {
+ MAM.registerPass([&] { return MMAHandle.getAnalysis(); });
+ PB.registerModuleAnalyses(MAM);
+ PB.registerCGSCCAnalyses(CGAM);
+ PB.registerFunctionAnalyses(FAM);
+ PB.registerLoopAnalyses(LAM);
+ PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
+ MPM.addPass(
+ RequireAnalysisPass<MockModuleAnalysisHandle::Analysis, Module>());
+ MPM.addPass(PGOInstrumentationGen());
+ }
+
+ void parseAssembly(const StringRef IR) {
+ SMDiagnostic Error;
+ M = parseAssemblyString(IR, Error, Context);
+ std::string ErrMsg;
+ raw_string_ostream OS(ErrMsg);
+ Error.print("", OS);
+
+ // A failure here means that the test itself is buggy.
+ if (!M)
+ report_fatal_error(OS.str().c_str());
+ }
+};
+
+TEST_F(PGOInstrumentationGenTest, NotInstrumentedDeclarationsOnly) {
+ const StringRef NotInstrumentableCode = R"(
+ declare i32 @f(i32);
+ )";
+
+ parseAssembly(NotInstrumentableCode);
+
+ ASSERT_THAT(M, NotNull());
+
+ 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, IsNull());
+}
+
+TEST_F(PGOInstrumentationGenTest, NotInstrumentedGlobals) {
+ const StringRef NotInstrumentableCode = R"(
+ @foo.table = internal unnamed_addr constant [1 x ptr] [ptr @f]
+ declare i32 @f(i32);
+ )";
+
+ parseAssembly(NotInstrumentableCode);
+
+ ASSERT_THAT(M, NotNull());
+
+ 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, IsNull());
+}
+
+TEST_F(PGOInstrumentationGenTest, Instrumented) {
+ const StringRef InstrumentableCode = R"(
+ define i32 @f(i32 %n) {
+ entry:
+ ret i32 0
+ }
+ )";
+
+ parseAssembly(InstrumentableCode);
+
+ 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());
+}
+
+} // end anonymous namespace
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
samolisov
force-pushed
the
pgo-instrument-when-at-least-one-definition
branch
from
4d46fc3 to
7bf25a1
Compare
| M, IntTy64, true, GlobalValue::WeakAnyLinkage, | ||
| Constant::getIntegerValue(IntTy64, APInt(64, ProfileVersion)), VarName); | ||
| Constant::getIntegerValue(IntTy64, APInt(64, ProfileVersion)), VarName, | ||
| InsertionPoint); |
There was a problem hiding this comment.
Why is the explicit insertion point needed now?
There was a problem hiding this comment.
I see the COMDATs are generated in an LLVM IR module at the beginning while globals are generated one after another. Because now the __llvm_profile_raw_version variable is generated after the counters, it is decoupled from the corresponding comdat directive by default (so, without the insertion point):
$__llvm_profile_raw_version = comdat any
@__profn_test_br_1 = private constant [9 x i8] c"test_br_1"
@__llvm_profile_raw_version = hidden constant i64 {00000}, comdatwhat looks ugly and may make the debugging process less smooth. This is why I added the explicit insertion point.
There was a problem hiding this comment.
this seems like a separate change that should be split out?
There was a problem hiding this comment.
@aeubanks do you mean the __llvm_profile_raw_version variable generation? If to separate only insertion point adding, some lit test must be modified temporarily and then returned back. I think this work isn't necessary, better I'll split the patch to the compile time optimization and the variable not generation. Thank you for the suggestion.
| @@ -0,0 +1,15 @@ | |||
| ; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN-COMDAT | |||
There was a problem hiding this comment.
How does this test behave different than the current status quo?
There was a problem hiding this comment.
The idea of the test is to check that the assert on line https://github.com/llvm/llvm-project/pull/93421/files#diff-618e0bf5dc7c44fae29843f3be69506a1b0a1409c7f018c61b0acf593f1c4605R408 is never triggered. A module with no global variables (other than the counters inserted during the instrumentation) was prepared to demonstrate that the version variable __llvm_profile_raw_version will be generated and it will be inserted in the module just after the corresponding comdat. If you think this is a redundant module, I'm going to remove it.
There was a problem hiding this comment.
if this is just for compile time optimization, then adding these tests isn't necessary
There was a problem hiding this comment.
ah the commit title is a bit misleading since there is a functional change in that we don't insert __llvm_profile_raw_version if there are no globals. so this test is fine
There was a problem hiding this comment.
@aeubanks Yes, unfortunately I have no idea how to reflect both changes in the title what is the sign there should be two commits.
llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
Outdated
Show resolved
Hide resolved
|
@teresajohnson Thank you very much for the feedback. I've answered inline and mark the PR as WIP because some rework is required. |
samolisov
changed the title
samolisov
changed the title
|
I'm going to rebase my changes onto updated main because an important part of the work: unit tests and the baseline for LIT tests are already there. I know, there is some troubles with rebased commits in GitHub UI, but I see no alternative. Maybe merging main into my branch but I afraid of a lot of intermediate commits will be displayed as changes in the PR and make the review impossible. |
samolisov
force-pushed
the
pgo-instrument-when-at-least-one-definition
branch
from
7bf25a1 to
37019cf
Compare
|
The PR has been modified to contain NFC changes only to optimize the compilation time. All the changes related to P.S. It seems, github doesn't allow to modify the source branch for a PR, so I cannot convert this PR 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 the second PR into that branch. There are some conflicts but I believe they will be solved after landing the changes from the current PR into main. This commit is reviewable: 5d5ead1 |
|
Gentle ping. |
llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp
Outdated
Show resolved
Hide resolved
|
@teresajohnson @aeubanks Could you have a look at the PR again? I believe, all the comments and suggestions were addressed. Even though the next parts of this "stacked" PR won't pass the review, this part may reduce the compilation time a little bit, at least. |
|
Gentle ping. |
samolisov
force-pushed
the
pgo-instrument-when-at-least-one-definition
branch
from
944d5f5 to
8cb224b
Compare
|
@teresajohnson This is my bad, I should have used the re-request review button. Anyway, thank you very much for review. I'm going to merge this after CI finishing. |
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.
samolisov
force-pushed
the
pgo-instrument-when-at-least-one-definition
branch
from
8cb224b to
9c08c66
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/7037 Here is the relevant piece of the build log for the reference |
|
Hi @samolisov Did you see that a buildbot failed with this patch? I see the same if I compile with EXPENSIVE_CHECKS set and run lit tests. They all hit the error |
Hello @mikaelholmen I saw this errors but unfortunately I cannot reproduce them locally with neither release nor debug builds. I have no idea what this expensive checks do and what is the difference with usual tests. Only thing what I see, the pass now may add a global variable without invalidating any analysis but I'm not sure is this a problem. If so, the better idea would be to revert the patch. |
Add to your cmake command and recompile and run lit tests. Then you should be able to see the failures. |
|
@mikaelholmen Thank you very much! I've added the flag and now can reproduce the issue. @teresajohnson @aeubanks Could you help? I opened the second PR to create the is still generated and this changes the hash of the module what leads to error emission in the PassManager: I see the following two ways: either to combine both patches into one or just to revert the current changes (what has already been done) to not brake the pipelines. So, I think we may discuss the changed focusing on whether the global variable |
…llvm#93421)" This reverts commit 23c64be.
…3421) 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.
…3421) 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.
…llvm#93421)" This reverts commit 23c64be.
…3421) 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.
…llvm#93421)" This reverts commit 23c64be.
The
PGOInstrumentationGenpass should preserve all analysis resultswhen 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.