From f6683009dc4465d67c6120ea41e04f136a7efe72 Mon Sep 17 00:00:00 2001 From: Michele Scandale Date: Sun, 17 Nov 2024 18:55:20 -0800 Subject: [PATCH] [Analysis] Remove global state from `PluginInline{Advisor,Order}Analysis`. The plugin analysis for `InlineAdvisor` and `InlineOrder` currently relies on shared global state to keep track if the analysis is available. This causes issues when pipelines using plugins and pipelines not using plugins are run in the same process. The shared global state can be easily replaced by checking in the given instance of `ModuleAnalysisManager` if the plugin analysis has been registered. --- llvm/include/llvm/Analysis/InlineAdvisor.h | 2 - llvm/include/llvm/Analysis/InlineOrder.h | 5 --- llvm/include/llvm/IR/PassManager.h | 12 ++++-- llvm/lib/Analysis/InlineAdvisor.cpp | 3 +- llvm/lib/Analysis/InlineOrder.cpp | 3 +- .../PluginInlineAdvisorAnalysisTest.cpp | 41 +++++-------------- .../PluginInlineOrderAnalysisTest.cpp | 6 --- 7 files changed, 20 insertions(+), 52 deletions(-) diff --git a/llvm/include/llvm/Analysis/InlineAdvisor.h b/llvm/include/llvm/Analysis/InlineAdvisor.h index 700c3b0f18b8d..b002bec2c9183 100644 --- a/llvm/include/llvm/Analysis/InlineAdvisor.h +++ b/llvm/include/llvm/Analysis/InlineAdvisor.h @@ -287,7 +287,6 @@ class PluginInlineAdvisorAnalysis : public AnalysisInfoMixin { public: static AnalysisKey Key; - static bool HasBeenRegistered; typedef InlineAdvisor *(*AdvisorFactory)(Module &M, FunctionAnalysisManager &FAM, @@ -295,7 +294,6 @@ class PluginInlineAdvisorAnalysis InlineContext IC); PluginInlineAdvisorAnalysis(AdvisorFactory Factory) : Factory(Factory) { - HasBeenRegistered = true; assert(Factory != nullptr && "The plugin advisor factory should not be a null pointer."); } diff --git a/llvm/include/llvm/Analysis/InlineOrder.h b/llvm/include/llvm/Analysis/InlineOrder.h index 2fa2d6091303a..498cef314b5c3 100644 --- a/llvm/include/llvm/Analysis/InlineOrder.h +++ b/llvm/include/llvm/Analysis/InlineOrder.h @@ -59,7 +59,6 @@ class PluginInlineOrderAnalysis ModuleAnalysisManager &MAM, Module &M); PluginInlineOrderAnalysis(InlineOrderFactory Factory) : Factory(Factory) { - HasBeenRegistered = true; assert(Factory != nullptr && "The plugin inline order factory should not be a null pointer."); } @@ -71,11 +70,7 @@ class PluginInlineOrderAnalysis Result run(Module &, ModuleAnalysisManager &) { return {Factory}; } Result getResult() { return {Factory}; } - static bool isRegistered() { return HasBeenRegistered; } - static void unregister() { HasBeenRegistered = false; } - private: - static bool HasBeenRegistered; InlineOrderFactory Factory; }; diff --git a/llvm/include/llvm/IR/PassManager.h b/llvm/include/llvm/IR/PassManager.h index d269221fac070..5dab9d0d0a797 100644 --- a/llvm/include/llvm/IR/PassManager.h +++ b/llvm/include/llvm/IR/PassManager.h @@ -398,6 +398,11 @@ template class AnalysisManager { AnalysisResultLists.clear(); } + /// Returns true if the specified analysis pass is registered. + template bool isPassRegistered() const { + return AnalysisPasses.count(PassT::ID()); + } + /// Get the result of an analysis pass for a given IR unit. /// /// Runs the analysis if a cached result is not available. @@ -458,10 +463,9 @@ template class AnalysisManager { /// and this function returns true. /// /// (Note: Although the return value of this function indicates whether or not - /// an analysis was previously registered, there intentionally isn't a way to - /// query this directly. Instead, you should just register all the analyses - /// you might want and let this class run them lazily. This idiom lets us - /// minimize the number of times we have to look up analyses in our + /// an analysis was previously registered, you should just register all the + /// analyses you might want and let this class run them lazily. This idiom + /// lets us minimize the number of times we have to look up analyses in our /// hashtable.) template bool registerPass(PassBuilderT &&PassBuilder) { diff --git a/llvm/lib/Analysis/InlineAdvisor.cpp b/llvm/lib/Analysis/InlineAdvisor.cpp index 45702fa25d8b1..12553dd446a61 100644 --- a/llvm/lib/Analysis/InlineAdvisor.cpp +++ b/llvm/lib/Analysis/InlineAdvisor.cpp @@ -199,13 +199,12 @@ void InlineAdvice::recordInliningWithCalleeDeleted() { AnalysisKey InlineAdvisorAnalysis::Key; AnalysisKey PluginInlineAdvisorAnalysis::Key; -bool PluginInlineAdvisorAnalysis::HasBeenRegistered = false; bool InlineAdvisorAnalysis::Result::tryCreate( InlineParams Params, InliningAdvisorMode Mode, const ReplayInlinerSettings &ReplaySettings, InlineContext IC) { auto &FAM = MAM.getResult(M).getManager(); - if (PluginInlineAdvisorAnalysis::HasBeenRegistered) { + if (MAM.isPassRegistered()) { auto &DA = MAM.getResult(M); Advisor.reset(DA.Factory(M, FAM, Params, IC)); return !!Advisor; diff --git a/llvm/lib/Analysis/InlineOrder.cpp b/llvm/lib/Analysis/InlineOrder.cpp index f156daa2f126f..8d920153f250d 100644 --- a/llvm/lib/Analysis/InlineOrder.cpp +++ b/llvm/lib/Analysis/InlineOrder.cpp @@ -283,7 +283,6 @@ class PriorityInlineOrder : public InlineOrder> { } // namespace AnalysisKey llvm::PluginInlineOrderAnalysis::Key; -bool llvm::PluginInlineOrderAnalysis::HasBeenRegistered; std::unique_ptr>> llvm::getDefaultInlineOrder(FunctionAnalysisManager &FAM, @@ -313,7 +312,7 @@ llvm::getDefaultInlineOrder(FunctionAnalysisManager &FAM, std::unique_ptr>> llvm::getInlineOrder(FunctionAnalysisManager &FAM, const InlineParams &Params, ModuleAnalysisManager &MAM, Module &M) { - if (llvm::PluginInlineOrderAnalysis::isRegistered()) { + if (MAM.isPassRegistered()) { LLVM_DEBUG(dbgs() << " Current used priority: plugin ---- \n"); return MAM.getResult(M).Factory(FAM, Params, MAM, M); diff --git a/llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp b/llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp index 3330751120e6c..92c0b1bcacb12 100644 --- a/llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp +++ b/llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp @@ -87,33 +87,10 @@ struct CompilerInstance { ThinOrFullLTOPhase::None)); } - ~CompilerInstance() { - // Reset the static variable that tracks if the plugin has been registered. - // This is needed to allow the test to run multiple times. - PluginInlineAdvisorAnalysis::HasBeenRegistered = false; - } - std::string output; std::unique_ptr outputM; - // run with the default inliner - auto run_default(StringRef IR) { - PluginInlineAdvisorAnalysis::HasBeenRegistered = false; - outputM = parseAssemblyString(IR, Error, Ctx); - MPM.run(*outputM, MAM); - ASSERT_TRUE(outputM); - output.clear(); - raw_string_ostream o_stream{output}; - outputM->print(o_stream, nullptr); - ASSERT_TRUE(true); - } - - // run with the dnamic inliner - auto run_dynamic(StringRef IR) { - // note typically the constructor for the DynamicInlineAdvisorAnalysis - // will automatically set this to true, we controll it here only to - // altenate between the default and dynamic inliner in our test - PluginInlineAdvisorAnalysis::HasBeenRegistered = true; + auto run(StringRef IR) { outputM = parseAssemblyString(IR, Error, Ctx); MPM.run(*outputM, MAM); ASSERT_TRUE(outputM); @@ -274,14 +251,16 @@ TEST(PluginInlineAdvisorTest, PluginLoad) { // Skip the test if plugins are disabled. GTEST_SKIP(); #endif - CompilerInstance CI{}; - CI.setupPlugin(); + CompilerInstance DefaultCI{}; + + CompilerInstance PluginCI{}; + PluginCI.setupPlugin(); for (StringRef IR : TestIRS) { - CI.run_default(IR); - std::string default_output = CI.output; - CI.run_dynamic(IR); - std::string dynamic_output = CI.output; + DefaultCI.run(IR); + std::string default_output = DefaultCI.output; + PluginCI.run(IR); + std::string dynamic_output = PluginCI.output; ASSERT_EQ(default_output, dynamic_output); } } @@ -294,7 +273,7 @@ TEST(PluginInlineAdvisorTest, CustomAdvisor) { CI.setupFooOnly(); for (StringRef IR : TestIRS) { - CI.run_dynamic(IR); + CI.run(IR); CallGraph CGraph = CallGraph(*CI.outputM); for (auto &node : CGraph) { for (auto &edge : *node.second) { diff --git a/llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp b/llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp index ca860a0dd5584..0b31b0892d75a 100644 --- a/llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp +++ b/llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp @@ -61,12 +61,6 @@ struct CompilerInstance { ThinOrFullLTOPhase::None)); } - ~CompilerInstance() { - // Reset the static variable that tracks if the plugin has been registered. - // This is needed to allow the test to run multiple times. - PluginInlineOrderAnalysis::unregister(); - } - std::string Output; std::unique_ptr OutputM;