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, 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); From 5d5ead1dbd9aac486aada3acf81cc09464ab7bae Mon Sep 17 00:00:00 2001 From: Pavel Samolysov Date: Fri, 31 May 2024 05:52:00 +0300 Subject: [PATCH 2/2] [PGO] Generate __llvm_profile_raw_version only when instrumented 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. --- .../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, 31 insertions(+), 19 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 fbf9688ed2d1e..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 LookupTLI, function_ref LookupBPI, function_ref 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,9 +1855,15 @@ 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 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 cbeaa501d4d88..01d17c5059a1a 100644 --- a/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp +++ b/llvm/unittests/Transforms/Instrumentation/PGOInstrumentationTest.cpp @@ -217,8 +217,7 @@ TEST_P(PGOInstrumentationGenIgnoreTest, NotInstrumented) { 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