[Analysis] Remove global state from PluginInline{Advisor,Order}Analysis.
#114615
Conversation
llvmbot
added
llvm:ir
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-ir Author: Michele Scandale (michele-scandale) ChangesThe plugin analysis for Full diff: https://github.com/llvm/llvm-project/pull/114615.diff 7 Files Affected:
diff --git a/llvm/include/llvm/Analysis/InlineAdvisor.h b/llvm/include/llvm/Analysis/InlineAdvisor.h
index 700c3b0f18b8d5..b002bec2c91836 100644
--- a/llvm/include/llvm/Analysis/InlineAdvisor.h
+++ b/llvm/include/llvm/Analysis/InlineAdvisor.h
@@ -287,7 +287,6 @@ class PluginInlineAdvisorAnalysis
: public AnalysisInfoMixin<PluginInlineAdvisorAnalysis> {
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 2fa2d6091303ad..498cef314b5c31 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 d269221fac0701..cd090384c353ec 100644
--- a/llvm/include/llvm/IR/PassManager.h
+++ b/llvm/include/llvm/IR/PassManager.h
@@ -398,6 +398,11 @@ template <typename IRUnitT, typename... ExtraArgTs> class AnalysisManager {
AnalysisResultLists.clear();
}
+ /// Returns true if the specified analysis pass is registered.
+ template <typename PassT> 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.
diff --git a/llvm/lib/Analysis/InlineAdvisor.cpp b/llvm/lib/Analysis/InlineAdvisor.cpp
index 45702fa25d8b14..12553dd446a616 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<FunctionAnalysisManagerModuleProxy>(M).getManager();
- if (PluginInlineAdvisorAnalysis::HasBeenRegistered) {
+ if (MAM.isPassRegistered<PluginInlineAdvisorAnalysis>()) {
auto &DA = MAM.getResult<PluginInlineAdvisorAnalysis>(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 f156daa2f126fb..8d920153f250df 100644
--- a/llvm/lib/Analysis/InlineOrder.cpp
+++ b/llvm/lib/Analysis/InlineOrder.cpp
@@ -283,7 +283,6 @@ class PriorityInlineOrder : public InlineOrder<std::pair<CallBase *, int>> {
} // namespace
AnalysisKey llvm::PluginInlineOrderAnalysis::Key;
-bool llvm::PluginInlineOrderAnalysis::HasBeenRegistered;
std::unique_ptr<InlineOrder<std::pair<CallBase *, int>>>
llvm::getDefaultInlineOrder(FunctionAnalysisManager &FAM,
@@ -313,7 +312,7 @@ llvm::getDefaultInlineOrder(FunctionAnalysisManager &FAM,
std::unique_ptr<InlineOrder<std::pair<CallBase *, int>>>
llvm::getInlineOrder(FunctionAnalysisManager &FAM, const InlineParams &Params,
ModuleAnalysisManager &MAM, Module &M) {
- if (llvm::PluginInlineOrderAnalysis::isRegistered()) {
+ if (MAM.isPassRegistered<PluginInlineOrderAnalysis>()) {
LLVM_DEBUG(dbgs() << " Current used priority: plugin ---- \n");
return MAM.getResult<PluginInlineOrderAnalysis>(M).Factory(FAM, Params, MAM,
M);
diff --git a/llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp b/llvm/unittests/Analysis/PluginInlineAdvisorAnalysisTest.cpp
index 3330751120e6c8..92c0b1bcacb12b 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<Module> 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 ca860a0dd55843..0b31b0892d75ae 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<Module> OutputM;
|
|
I agree that global state breaks a bunch of cases that we care about, but using the pass manager infrastructure to decide whether or not to use the default vs ML inline advisor seems like a hack to avoid having to create the proper APIs. IMO each inliner instance should decide if it's going to use the ML inliner or not, not each pipeline. Thoughts? |
I believe the proposed change is in line with the original change that introduced the
Today the |
|
I see some comments at about adding a pass to register an analysis. Is there any reason we can't just register the analysis directly withPassBuilder::registerAnalysisRegistrationCallback in llvmGetPassPluginInfo? that seems like it'd solve your issue IIUC?
|
In the context I'm working on there are multiple independent optimization pipelines in the same process for different purposes (each pipeline has its own analysis manager instances): for some of them this exact mechanism is being used (due to a static plugin being registered) to customize the inline advisor, and that end up breaking the other pipelines because of the shared global state. |
IIUC you only want some of the pipelines in the process to use the custom inline advisor but not others? I'm not understanding how this patch does better than |
I think I'm not understanding how with this patch applied is any different from just with the |
The problem is on the the |
|
thanks for pointing out that specific piece of code, I can't come up with a better way of doing that so this PR lgtm. please update the comments here in this PR, removing the part where it says there's no way to query if an analysis is registered I'd also request that the comments here be updated to use |
michele-scandale
force-pushed
the
remove-global-state-plugin-analysis-inliner
branch
from
3d3e5d6 to
aee6944
Compare
| /// | ||
| /// (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 |
There was a problem hiding this comment.
please keep the bit about just registering analyses without checking if they're already registered
There was a problem hiding this comment.
Something like "Although the return value of this function indicates whether or not
/// an analysis was previously registered, you should just register all the analyses..."
…sis`. 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.
michele-scandale
force-pushed
the
remove-global-state-plugin-analysis-inliner
branch
from
aee6944 to
f668300
Compare
The plugin analysis for
InlineAdvisorandInlineOrdercurrently 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
ModuleAnalysisManagerif the plugin analysis has been registered.