[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.
llvm · May 30, 2024 · 37019cf · 37019cf
1 parent 0a93e9f
commit 37019cf
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 36 deletions.
diff --git 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,14 +1845,25 @@ static bool InstrumentAllFunctions(
   std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
   collectComdatMembers(M, ComdatMembers);
 
+  bool AnythingInstrumented = false;
   for (auto &F : M) {
     if (skipPGOGen(F))
       continue;
     auto &TLI = LookupTLI(F);
     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
@@ -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
@@ -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
@@ -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
@@ -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