[CodeGen][NewPM] Port RegAllocEvictionAdvisor analysis to NPM #117309
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
2 times, most recently
from
50b7ba3 to
0066231
Compare
|
@llvm/pr-subscribers-llvm-regalloc @llvm/pr-subscribers-mlgo Author: Akshat Oke (optimisan) ChangesLegacy pass used to provide the advisor, so this extracts that logic into a provider class used by both analysis passes. All three (Default, Release, Development) legacy passes There is only one NPM analysis Also adds Patch is 28.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117309.diff 10 Files Affected:
diff --git a/llvm/lib/CodeGen/RegAllocEvictionAdvisor.h b/llvm/include/llvm/CodeGen/RegAllocEvictionAdvisor.h
similarity index 75%
rename from llvm/lib/CodeGen/RegAllocEvictionAdvisor.h
rename to llvm/include/llvm/CodeGen/RegAllocEvictionAdvisor.h
index 52dd946a685400..847bf032235c1d 100644
--- a/llvm/lib/CodeGen/RegAllocEvictionAdvisor.h
+++ b/llvm/include/llvm/CodeGen/RegAllocEvictionAdvisor.h
@@ -9,11 +9,13 @@
#ifndef LLVM_CODEGEN_REGALLOCEVICTIONADVISOR_H
#define LLVM_CODEGEN_REGALLOCEVICTIONADVISOR_H
+#include "llvm/ADT/Any.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/CodeGen/Register.h"
#include "llvm/Config/llvm-config.h"
+#include "llvm/IR/PassManager.h"
#include "llvm/MC/MCRegister.h"
#include "llvm/Pass.h"
@@ -164,12 +166,12 @@ class RegAllocEvictionAdvisor {
///
/// Because we need to offer additional services in 'development' mode, the
/// implementations of this analysis need to implement RTTI support.
-class RegAllocEvictionAdvisorAnalysis : public ImmutablePass {
+class RegAllocEvictionAdvisorAnalysisLegacy : public ImmutablePass {
public:
enum class AdvisorMode : int { Default, Release, Development };
- RegAllocEvictionAdvisorAnalysis(AdvisorMode Mode)
- : ImmutablePass(ID), Mode(Mode){};
+ RegAllocEvictionAdvisorAnalysisLegacy(AdvisorMode Mode)
+ : ImmutablePass(ID), Mode(Mode) {};
static char ID;
/// Get an advisor for the given context (i.e. machine function, etc)
@@ -177,7 +179,7 @@ class RegAllocEvictionAdvisorAnalysis : public ImmutablePass {
getAdvisor(const MachineFunction &MF, const RAGreedy &RA) = 0;
AdvisorMode getAdvisorMode() const { return Mode; }
virtual void logRewardIfNeeded(const MachineFunction &MF,
- llvm::function_ref<float()> GetReward){};
+ llvm::function_ref<float()> GetReward) {};
protected:
// This analysis preserves everything, and subclasses may have additional
@@ -191,13 +193,66 @@ class RegAllocEvictionAdvisorAnalysis : public ImmutablePass {
const AdvisorMode Mode;
};
+/// Common provider for legacy and new pass managers.
+/// This keeps the state for logging, and sets up and holds the provider.
+/// The legacy pass itself used to keep the logging state and provider,
+/// so this extraction helps the NPM analysis to reuse the logic.
+class RegAllocEvictionAdvisorProvider {
+public:
+ enum class AdvisorMode : int { Default, Release, Development };
+ RegAllocEvictionAdvisorProvider(AdvisorMode Mode) : Mode(Mode) {}
+
+ virtual ~RegAllocEvictionAdvisorProvider() = default;
+
+ virtual bool doInitialization(Module &M) { return false; }
+
+ virtual void logRewardIfNeeded(const MachineFunction &MF,
+ llvm::function_ref<float()> GetReward) {}
+
+ virtual std::unique_ptr<RegAllocEvictionAdvisor>
+ getAdvisor(const MachineFunction &MF, const RAGreedy &RA) = 0;
+
+ /// Set the analyses that the advisor needs to use as they might not be
+ /// available before the advisor is created.
+ virtual void setAnalyses(std::initializer_list<llvm::Any> AnalysisP) {}
+
+ AdvisorMode getAdvisorMode() const { return Mode; }
+
+private:
+ const AdvisorMode Mode;
+};
+
+RegAllocEvictionAdvisorProvider *createReleaseModeAdvisorProvider();
+RegAllocEvictionAdvisorProvider *createDevelopmentModeAdvisorProvider();
+
+/// A Module analysis for fetching the Eviction Advisor. This is not a
+/// MachineFunction analysis for two reasons:
+/// - in the ML implementation case, the evaluator is stateless but (especially
+/// in the development mode) expensive to set up. With a Module Analysis, we
+/// `require` it and set it up once.
+/// - in the 'development' mode ML case, we want to capture the training log
+/// during allocation (this is a log of features encountered and decisions
+/// made), and then measure a score, potentially a few steps after allocation
+/// completes. So we need a Module analysis to keep the logger state around
+/// until we can make that measurement.
+class RegAllocEvictionAdvisorAnalysis
+ : public AnalysisInfoMixin<RegAllocEvictionAdvisorAnalysis> {
+ static AnalysisKey Key;
+ friend AnalysisInfoMixin<RegAllocEvictionAdvisorAnalysis>;
+
+public:
+ using Result = std::unique_ptr<RegAllocEvictionAdvisorProvider>;
+ Result run(Module &MF, ModuleAnalysisManager &MAM);
+};
+
/// Specialization for the API used by the analysis infrastructure to create
/// an instance of the eviction advisor.
-template <> Pass *callDefaultCtor<RegAllocEvictionAdvisorAnalysis>();
+template <> Pass *callDefaultCtor<RegAllocEvictionAdvisorAnalysisLegacy>();
-RegAllocEvictionAdvisorAnalysis *createReleaseModeAdvisor();
+RegAllocEvictionAdvisorAnalysisLegacy *createReleaseModeAdvisorAnalysisLegacy();
-RegAllocEvictionAdvisorAnalysis *createDevelopmentModeAdvisor();
+RegAllocEvictionAdvisorAnalysisLegacy *
+createDevelopmentModeAdvisorAnalysisLegacy();
// TODO: move to RegAllocEvictionAdvisor.cpp when we move implementation
// out of RegAllocGreedy.cpp
diff --git a/llvm/include/llvm/InitializePasses.h b/llvm/include/llvm/InitializePasses.h
index af93d5e989f654..19d64d34513a05 100644
--- a/llvm/include/llvm/InitializePasses.h
+++ b/llvm/include/llvm/InitializePasses.h
@@ -252,7 +252,7 @@ void initializePseudoProbeInserterPass(PassRegistry &);
void initializeRAGreedyPass(PassRegistry &);
void initializeReachingDefAnalysisPass(PassRegistry &);
void initializeReassociateLegacyPassPass(PassRegistry &);
-void initializeRegAllocEvictionAdvisorAnalysisPass(PassRegistry &);
+void initializeRegAllocEvictionAdvisorAnalysisLegacyPass(PassRegistry &);
void initializeRegAllocFastPass(PassRegistry &);
void initializeRegAllocPriorityAdvisorAnalysisPass(PassRegistry &);
void initializeRegAllocScoringPass(PassRegistry &);
diff --git a/llvm/include/llvm/Passes/CodeGenPassBuilder.h b/llvm/include/llvm/Passes/CodeGenPassBuilder.h
index d2e9e8185a2b90..aa6437c1d2f017 100644
--- a/llvm/include/llvm/Passes/CodeGenPassBuilder.h
+++ b/llvm/include/llvm/Passes/CodeGenPassBuilder.h
@@ -54,6 +54,7 @@
#include "llvm/CodeGen/PHIElimination.h"
#include "llvm/CodeGen/PeepholeOptimizer.h"
#include "llvm/CodeGen/PreISelIntrinsicLowering.h"
+#include "llvm/CodeGen/RegAllocEvictionAdvisor.h"
#include "llvm/CodeGen/RegAllocFast.h"
#include "llvm/CodeGen/RegUsageInfoCollector.h"
#include "llvm/CodeGen/RegUsageInfoPropagate.h"
@@ -1061,9 +1062,10 @@ void CodeGenPassBuilder<Derived, TargetMachineT>::addMachineSSAOptimization(
template <typename Derived, typename TargetMachineT>
void CodeGenPassBuilder<Derived, TargetMachineT>::addTargetRegisterAllocator(
AddMachinePass &addPass, bool Optimized) const {
- if (Optimized)
+ if (Optimized) {
+ addPass(RequireAnalysis<RegAllocEvictionAdvisorAnalysis>());
addPass(RAGreedyPass());
- else
+ } else
addPass(RegAllocFastPass());
}
diff --git a/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp b/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
index d099544c2a4918..9c15c327bb202c 100644
--- a/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
+++ b/llvm/lib/CodeGen/MLRegAllocEvictAdvisor.cpp
@@ -11,11 +11,11 @@
//===----------------------------------------------------------------------===//
#include "AllocationOrder.h"
-#include "RegAllocEvictionAdvisor.h"
#include "RegAllocGreedy.h"
#include "llvm/Analysis/InteractiveModelRunner.h"
#include "llvm/Analysis/MLModelRunner.h"
#include "llvm/Analysis/TensorSpec.h"
+#include "llvm/CodeGen/RegAllocEvictionAdvisor.h"
#if defined(LLVM_HAVE_TF_AOT_REGALLOCEVICTMODEL) || defined(LLVM_HAVE_TFLITE)
#include "llvm/Analysis/ModelUnderTrainingRunner.h"
#include "llvm/Analysis/NoInferenceModelRunner.h"
@@ -108,7 +108,7 @@ class RegAllocScoring : public MachineFunctionPass {
/// RegAllocReward analysis usage.
void getAnalysisUsage(AnalysisUsage &AU) const override {
AU.setPreservesAll();
- AU.addRequired<RegAllocEvictionAdvisorAnalysis>();
+ AU.addRequired<RegAllocEvictionAdvisorAnalysisLegacy>();
AU.addRequired<RegAllocPriorityAdvisorAnalysis>();
AU.addRequired<MachineBlockFrequencyInfoWrapperPass>();
MachineFunctionPass::getAnalysisUsage(AU);
@@ -366,11 +366,12 @@ class MLEvictAdvisor : public RegAllocEvictionAdvisor {
// ===================================
// Release (AOT) - specifics
// ===================================
-class ReleaseModeEvictionAdvisorAnalysis final
- : public RegAllocEvictionAdvisorAnalysis {
+/// Common provider for legacy and new pass managers.
+class ReleaseModeEvictionAdvisorProvider final
+ : public RegAllocEvictionAdvisorProvider {
public:
- ReleaseModeEvictionAdvisorAnalysis()
- : RegAllocEvictionAdvisorAnalysis(AdvisorMode::Release) {
+ ReleaseModeEvictionAdvisorProvider()
+ : RegAllocEvictionAdvisorProvider(AdvisorMode::Release) {
if (EnableDevelopmentFeatures) {
InputFeatures = {RA_EVICT_FEATURES_LIST(
_DECL_FEATURES) RA_EVICT_FIRST_DEVELOPMENT_FEATURE(_DECL_FEATURES)
@@ -380,17 +381,17 @@ class ReleaseModeEvictionAdvisorAnalysis final
}
}
// support for isa<> and dyn_cast.
- static bool classof(const RegAllocEvictionAdvisorAnalysis *R) {
+ static bool classof(const RegAllocEvictionAdvisorProvider *R) {
return R->getAdvisorMode() == AdvisorMode::Release;
}
-private:
- std::vector<TensorSpec> InputFeatures;
-
- void getAnalysisUsage(AnalysisUsage &AU) const override {
- AU.addRequired<MachineBlockFrequencyInfoWrapperPass>();
- AU.addRequired<MachineLoopInfoWrapperPass>();
- RegAllocEvictionAdvisorAnalysis::getAnalysisUsage(AU);
+ void setAnalyses(std::initializer_list<llvm::Any> AnalysisList) override {
+ for (auto Analysis : AnalysisList) {
+ if (auto **MBFI = llvm::any_cast<MachineBlockFrequencyInfo *>(&Analysis))
+ this->MBFI = *MBFI;
+ if (auto **Loops = llvm::any_cast<MachineLoopInfo *>(&Analysis))
+ this->Loops = *Loops;
+ }
}
std::unique_ptr<RegAllocEvictionAdvisor>
@@ -405,12 +406,47 @@ class ReleaseModeEvictionAdvisorAnalysis final
InteractiveChannelBaseName + ".out",
InteractiveChannelBaseName + ".in");
}
- return std::make_unique<MLEvictAdvisor>(
- MF, RA, Runner.get(),
- getAnalysis<MachineBlockFrequencyInfoWrapperPass>().getMBFI(),
- getAnalysis<MachineLoopInfoWrapperPass>().getLI());
+ return std::make_unique<MLEvictAdvisor>(MF, RA, Runner.get(), *MBFI,
+ *Loops);
}
+
+private:
+ std::vector<TensorSpec> InputFeatures;
std::unique_ptr<MLModelRunner> Runner;
+ MachineBlockFrequencyInfo *MBFI;
+ MachineLoopInfo *Loops;
+};
+
+class ReleaseModeEvictionAdvisorAnalysisLegacy final
+ : public RegAllocEvictionAdvisorAnalysisLegacy {
+public:
+ ReleaseModeEvictionAdvisorAnalysisLegacy()
+ : RegAllocEvictionAdvisorAnalysisLegacy(AdvisorMode::Release) {}
+
+ std::unique_ptr<RegAllocEvictionAdvisor>
+ getAdvisor(const MachineFunction &MF, const RAGreedy &RA) override {
+ auto *MBFI = &getAnalysis<MachineBlockFrequencyInfoWrapperPass>().getMBFI();
+ auto *Loops = &getAnalysis<MachineLoopInfoWrapperPass>().getLI();
+ Provider.setAnalyses({MBFI, Loops});
+ return Provider.getAdvisor(MF, RA);
+ }
+
+ bool doInitialization(Module &M) override {
+ return Provider.doInitialization(M);
+ }
+
+ static bool classof(const RegAllocEvictionAdvisorAnalysisLegacy *R) {
+ return R->getAdvisorMode() == AdvisorMode::Release;
+ }
+
+ void getAnalysisUsage(AnalysisUsage &AU) const override {
+ AU.addRequired<MachineBlockFrequencyInfoWrapperPass>();
+ AU.addRequired<MachineLoopInfoWrapperPass>();
+ RegAllocEvictionAdvisorAnalysisLegacy::getAnalysisUsage(AU);
+ }
+
+private:
+ ReleaseModeEvictionAdvisorProvider Provider;
};
// ===================================
@@ -445,11 +481,14 @@ class DevelopmentModeEvictAdvisor : public MLEvictAdvisor {
Logger *const Log;
};
-class DevelopmentModeEvictionAdvisorAnalysis final
- : public RegAllocEvictionAdvisorAnalysis {
+class DevelopmentModeEvictionAdvisorProvider final
+ : public RegAllocEvictionAdvisorProvider {
public:
- DevelopmentModeEvictionAdvisorAnalysis()
- : RegAllocEvictionAdvisorAnalysis(AdvisorMode::Development) {
+ DevelopmentModeEvictionAdvisorProvider(
+ MachineBlockFrequencyInfo *MBFI = nullptr,
+ MachineLoopInfo *Loops = nullptr)
+ : RegAllocEvictionAdvisorProvider(AdvisorMode::Development), MBFI(MBFI),
+ Loops(Loops) {
if (EnableDevelopmentFeatures) {
InputFeatures = {RA_EVICT_FEATURES_LIST(
_DECL_FEATURES) RA_EVICT_FIRST_DEVELOPMENT_FEATURE(_DECL_FEATURES)
@@ -471,7 +510,7 @@ class DevelopmentModeEvictionAdvisorAnalysis final
}
}
// support for isa<> and dyn_cast.
- static bool classof(const RegAllocEvictionAdvisorAnalysis *R) {
+ static bool classof(const RegAllocEvictionAdvisorProvider *R) {
return R->getAdvisorMode() == AdvisorMode::Development;
}
@@ -491,14 +530,13 @@ class DevelopmentModeEvictionAdvisorAnalysis final
Log->logReward<float>(GetReward());
}
-private:
- std::vector<TensorSpec> InputFeatures;
- std::vector<TensorSpec> TrainingInputFeatures;
-
- void getAnalysisUsage(AnalysisUsage &AU) const override {
- AU.addRequired<MachineBlockFrequencyInfoWrapperPass>();
- AU.addRequired<MachineLoopInfoWrapperPass>();
- RegAllocEvictionAdvisorAnalysis::getAnalysisUsage(AU);
+ void setAnalyses(std::initializer_list<llvm::Any> AnalysisList) override {
+ for (auto Analysis : AnalysisList) {
+ if (auto **MBFI = llvm::any_cast<MachineBlockFrequencyInfo *>(&Analysis))
+ this->MBFI = *MBFI;
+ if (auto **Loops = llvm::any_cast<MachineLoopInfo *>(&Analysis))
+ this->Loops = *Loops;
+ }
}
bool doInitialization(Module &M) override {
@@ -544,14 +582,53 @@ class DevelopmentModeEvictionAdvisorAnalysis final
return nullptr;
if (Log)
Log->switchContext(MF.getName());
+ assert((MBFI && Loops) &&
+ "Invalid provider state: must have analysis available");
return std::make_unique<DevelopmentModeEvictAdvisor>(
- MF, RA, Runner.get(),
- getAnalysis<MachineBlockFrequencyInfoWrapperPass>().getMBFI(),
- getAnalysis<MachineLoopInfoWrapperPass>().getLI(), Log.get());
+ MF, RA, Runner.get(), *MBFI, *Loops, Log.get());
}
+private:
+ std::vector<TensorSpec> InputFeatures;
+ std::vector<TensorSpec> TrainingInputFeatures;
+
std::unique_ptr<MLModelRunner> Runner;
std::unique_ptr<Logger> Log;
+ const MachineBlockFrequencyInfo *MBFI;
+ const MachineLoopInfo *Loops;
+};
+
+class DevelopmentModeEvictionAdvisorAnalysisLegacy final
+ : public RegAllocEvictionAdvisorAnalysisLegacy {
+public:
+ DevelopmentModeEvictionAdvisorAnalysisLegacy()
+ : RegAllocEvictionAdvisorAnalysisLegacy(AdvisorMode::Development) {}
+
+ bool doInitialization(Module &M) override {
+ auto *MBFI = &getAnalysis<MachineBlockFrequencyInfoWrapperPass>().getMBFI();
+ auto *Loops = &getAnalysis<MachineLoopInfoWrapperPass>().getLI();
+ Provider.setAnalyses({MBFI, Loops});
+ return Provider.doInitialization(M);
+ }
+
+ std::unique_ptr<RegAllocEvictionAdvisor>
+ getAdvisor(const MachineFunction &MF, const RAGreedy &RA) override {
+ return Provider.getAdvisor(MF, RA);
+ }
+
+ // support for isa<> and dyn_cast.
+ static bool classof(const RegAllocEvictionAdvisorAnalysisLegacy *R) {
+ return R->getAdvisorMode() == AdvisorMode::Development;
+ }
+
+ void getAnalysisUsage(AnalysisUsage &AU) const override {
+ AU.addRequired<MachineBlockFrequencyInfoWrapperPass>();
+ AU.addRequired<MachineLoopInfoWrapperPass>();
+ RegAllocEvictionAdvisorAnalysisLegacy::getAnalysisUsage(AU);
+ }
+
+private:
+ DevelopmentModeEvictionAdvisorProvider Provider;
};
#endif //#ifdef LLVM_HAVE_TFLITE
@@ -1079,8 +1156,13 @@ void llvm::extractMBBFrequency(
// Development mode-specific implementations
#ifdef LLVM_HAVE_TFLITE
-RegAllocEvictionAdvisorAnalysis *llvm::createDevelopmentModeAdvisor() {
- return new DevelopmentModeEvictionAdvisorAnalysis();
+RegAllocEvictionAdvisorProvider *llvm::createDevelopmentModeAdvisorProvider() {
+ return new DevelopmentModeEvictionAdvisorProvider();
+}
+
+RegAllocEvictionAdvisorAnalysisLegacy *
+llvm::createDevelopmentModeAdvisorAnalysisLegacy() {
+ return new DevelopmentModeEvictionAdvisorAnalysisLegacy();
}
int64_t DevelopmentModeEvictAdvisor::tryFindEvictionCandidatePosition(
@@ -1146,18 +1228,23 @@ bool RegAllocScoring::runOnMachineFunction(MachineFunction &MF) {
return *CachedReward;
};
- getAnalysis<RegAllocEvictionAdvisorAnalysis>().logRewardIfNeeded(MF,
- GetReward);
+ getAnalysis<RegAllocEvictionAdvisorAnalysisLegacy>().logRewardIfNeeded(
+ MF, GetReward);
getAnalysis<RegAllocPriorityAdvisorAnalysis>().logRewardIfNeeded(MF,
GetReward);
return false;
}
#endif // #ifdef LLVM_HAVE_TFLITE
-RegAllocEvictionAdvisorAnalysis *llvm::createReleaseModeAdvisor() {
+RegAllocEvictionAdvisorProvider *llvm::createReleaseModeAdvisorProvider() {
+ return new ReleaseModeEvictionAdvisorProvider();
+}
+
+RegAllocEvictionAdvisorAnalysisLegacy *
+llvm::createReleaseModeAdvisorAnalysisLegacy() {
return llvm::isEmbeddedModelEvaluatorValid<CompiledModelType>() ||
!InteractiveChannelBaseName.empty()
- ? new ReleaseModeEvictionAdvisorAnalysis()
+ ? new ReleaseModeEvictionAdvisorAnalysisLegacy()
: nullptr;
}
diff --git a/llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp b/llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
index a1f441ebd0d5e4..09fb5df259a97e 100644
--- a/llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
+++ b/llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp
@@ -10,9 +10,10 @@
//
//===----------------------------------------------------------------------===//
-#include "RegAllocEvictionAdvisor.h"
+#include "llvm/CodeGen/RegAllocEvictionAdvisor.h"
#include "AllocationOrder.h"
#include "RegAllocGreedy.h"
+#include "RegAllocPriorityAdvisor.h"
#include "llvm/CodeGen/LiveRegMatrix.h"
#include "llvm/CodeGen/MachineFunction.h"
#include "llvm/CodeGen/RegisterClassInfo.h"
@@ -26,17 +27,18 @@
using namespace llvm;
-static cl::opt<RegAllocEvictionAdvisorAnalysis::AdvisorMode> Mode(
+static cl::opt<RegAllocEvictionAdvisorAnalysisLegacy::AdvisorMode> Mode(
"regalloc-enable-advisor", cl::Hidden,
- cl::init(RegAllocEvictionAdvisorAnalysis::AdvisorMode::Default),
+ cl::init(RegAllocEvictionAdvisorAnalysisLegacy::AdvisorMode::Default),
cl::desc("Enable regalloc advisor mode"),
cl::values(
- clEnumValN(RegAllocEvictionAdvisorAnalysis::AdvisorMode::Default,
+ clEnumValN(RegAllocEvictionAdvisorAnalysisLegacy::AdvisorMode::Default,
"default", "Default"),
- clEnumValN(RegAllocEvictionAdvisorAnalysis::AdvisorMode::Release,
+ clEnumValN(RegAllocEvictionAdvisorAnalysisLegacy::AdvisorMode::Release,
"release", "precompiled"),
- clEnumValN(RegAllocEvictionAdvisorAnalysis::AdvisorMode::Development,
- "development", "for training")));
+ clEnumValN(
+ RegAllocEvictionAdvisorAnalysisLegacy::AdvisorMode::Development,
+ "development", "for training")));
static cl::opt<bool> EnableLocalReassignment(
"enable-local-reassign", cl::Hidden,
@@ -59,59 +61,114 @@ cl::opt<unsigned> EvictInterferenceCutoff(
#define LLVM_HAVE_TF_AOT
#endif
-char RegAllocEvictionAdvisorAnalysis::ID = 0;
-INITIALIZE_PASS(RegAllocEvictionAdvisorAnalysis, "regalloc-evict",
+char RegAllocEvictionAdvisorAnalysisLegacy::ID = 0;
+INITIALIZE_PASS(RegAllocEvictionAdvisorAnalysisLegacy, "regalloc-evict",
"Regalloc eviction policy", false, true)
namespace {
-class DefaultEvictionAdvisorAnalysis final
- : public RegAllocEvictionAdvisorAnalysis {
+class DefaultEvictionAdvisorProvider final
+ : public RegAllocEvictionAdvisorProvider {
public:
- DefaultEvictionAdvisorAnalysis(bool NotAsRequested)
- : RegAllocEvictionAdvisorAnalysis(AdvisorMode::Default),
+ DefaultEvictionAdvisorProvider(bool NotAsRequested)
+ : RegAllocEvictionAdvisorProvider(Adv...
[truncated]
|
|
In general we should avoid module analysis results here. I browsed the relevant codes here, some machine learning based advisors need to load external models so they need |
|
I'm trying to understand the goal here: is the intention to move codegen passes to NPM? I'd imagine there is a RFC for that, is this it? https://discourse.llvm.org/t/rfc-introducing-classes-for-the-codegen-driven-by-new-pass-manager/55877 I'm assuming you tried the change with the ML bits enabled? I.e. like the ml-opt-{dev|rel|devrel}-x86-64 bots do? (I'd appreciate waiting for a LGTM from @boomanaiden154 or me, but please note next week is a generally a slow week in the US because the end of it is a bank holiday. I am out, but will pick this up the Monday after) |
Yes, the actual pass doesn't matter. Every pass must be mechanically ported
You cannot yet codegen to the end. This is part of getting to a functional new pass manager invocation, there's nothing to test at this point |
|
Yes |
OK, and to confirm, the RFC for that is the one I referenced?
|
|
What I'm trying to understand is how the design is expected to evolve. Is the current patch a crutch that enables then making RAGreedy NPM-enabled, and then we can drop the need for legacy support from the Regalloc advisor, remove that provider, remove LPM-isms like (I read through the RFC but couldn't figure out what the approach to porting analyses was, sorry if I missed it) |
Yes, @paperchalice picked up those patches a while ago.
It's the same as porting IR passes to new PM. Both NewPM and legacy pass manager paths will need to coexist for the foreseeable future. So for now this is just additive, but someday the OldPM path will be deleted.
Yes |
|
Thanks. This really helps! FWIW, it'd help if the RFC was updated to clarify these things (like 1 sentence like above brings a lot of clarity!), and if it were linked in these patches. I really like that we're moving off LPM, my concerns were the ones that got addressed above (i.e. progression/evolution of design). I should be able to review today. Thanks! |
| const AdvisorMode Mode; | ||
| }; | ||
|
|
||
| RegAllocEvictionAdvisorProvider *createReleaseModeAdvisorProvider(); |
There was a problem hiding this comment.
do we need these create methods? Are they just an artifact that's not applicable anymore, coming from the LPM conversion?
There was a problem hiding this comment.
In fact, even more - I think you can drop the doInitialization from the providers and just pass the module in the ctor, since - iiuc - the create... functions are called in places you have a Module. In the LPM analyses' case, you can model the fields as std::unique_ptrs and dynamically initialze them there, wdyt?
There was a problem hiding this comment.
Can drop doInitialization, not sure how to work around declaring the create..Advisor()'s
The ML and Release mode providers are in MLRegAlloc..cpp so I wanted to expose only the "base" signature to create those providers in this default impl file RegAllocEvictionAdvisor.cpp
There was a problem hiding this comment.
Can't the respective analyses just instantiate the appropriate provider at their construction?
There was a problem hiding this comment.
They do for the legacy analyses, but these are only used for the new (and only one) analysis.
Without the methods, the new PM analysis has to be in MLRegAlloc..cpp to keep the ML advisors local there, and also have to move the Default Provider out to llvm::.
Or - just separate out the ML advisor initialization in the new analysis to the MLRegAlloc..cpp?
There was a problem hiding this comment.
Or - just separate out the ML advisor initialization in the new analysis to the
MLRegAlloc..cpp?
Yes, each analysis would instantiate its provider as its implementation detail.
There was a problem hiding this comment.
Removed them, separated out the ML initialization in the new PM analysis class to the ML implementation file.
|
I post a reply in RFC to summarize the work up to now: https://discourse.llvm.org/t/rfc-introducing-classes-for-the-codegen-driven-by-new-pass-manager/55877/26 |
Could you explain why to avoid module analysis results? |
Just want to avoid outer analysis proxy here and let machine function analysis manager manage the advisors.
If I haven't missed anything, the context here is used only for error handling, we can handle them in pass builder. |
|
Should I pass in the context from NewPMDriver into |
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
from
0066231 to
6faee3f
Compare
|
Updated the NPM analysis to be a MachineFunction analysis.
The provider is the analysis result rather than the |
| MF, RA, Runner.get(), | ||
| getAnalysis<MachineBlockFrequencyInfoWrapperPass>().getMBFI(), | ||
| getAnalysis<MachineLoopInfoWrapperPass>().getLI(), Log.get()); | ||
| MF, RA, Runner.get(), *MBFI, *Loops, Log.get()); |
There was a problem hiding this comment.
Why isn't this std::move(Runner)?
There was a problem hiding this comment.
Can change that in another patch
| // contexts. At this point, it'd be an error | ||
| if (Log->currentContext() != MF.getName()) { | ||
| MF.getFunction().getContext().emitError( | ||
| "The training log context shouldn't have had changed."); |
There was a problem hiding this comment.
Don't use contractions in error messages (but this is probably preexisting)
| // function, so we assume the last context belongs to this function. If | ||
| // this invariant ever changes, we can implement at that time switching | ||
| // contexts. At this point, it'd be an error | ||
| if (Log->currentContext() != MF.getName()) { |
There was a problem hiding this comment.
Relying on function names isn't great, there are anonymous functions...
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
from
dc7b8b1 to
364cfd1
Compare
optimisan
changed the base branch from
main
to
users/Akshat-Oke/12-19-_support_recycler_implement_move_constructor
optimisan
force-pushed
the
users/Akshat-Oke/12-19-_support_recycler_implement_move_constructor
branch
from
30e38c2 to
f5baddf
Compare
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
from
364cfd1 to
a1f8cf8
Compare
optimisan
force-pushed
the
users/Akshat-Oke/12-19-_support_recycler_implement_move_constructor
branch
from
f5baddf to
af2bec8
Compare
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
from
a1f8cf8 to
65f69e2
Compare
optimisan
force-pushed
the
users/Akshat-Oke/12-19-_support_recycler_implement_move_constructor
branch
from
af2bec8 to
e6fab53
Compare
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
from
65f69e2 to
220fd47
Compare
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
from
3ea391f to
234a0ec
Compare
| if (Mode == RegAllocEvictionAdvisorAnalysisLegacy::AdvisorMode::Default) | ||
| Provider.reset( | ||
| new DefaultEvictionAdvisorProvider(/*NotAsRequested=*/false, Ctx)); | ||
| else | ||
| initializeMLProvider(Mode, Ctx); | ||
| if (!Provider) | ||
| Provider.reset( | ||
| new DefaultEvictionAdvisorProvider(/*NotAsRequested=*/true, Ctx)); |
There was a problem hiding this comment.
I'm confused by the flow here. initializeMLProvider seems to be doing the exact same thing
There was a problem hiding this comment.
Idea was to separate the ML implementation and take it out from this general file. This used to have two free functions like createML..Provider()
There was a problem hiding this comment.
I don't see what's separated all. I'd rather just have duplicated code than whatever this is
There was a problem hiding this comment.
Reverting to one switch case
| new DefaultEvictionAdvisorProvider(/*NotAsRequested=*/false, Ctx)); | ||
| else | ||
| initializeMLProvider(Mode, Ctx); | ||
| if (!Provider) |
There was a problem hiding this comment.
This case would only be reachable under the else path, but the whole function is reimplemented in initializeMLProvider?
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
from
234a0ec to
72eff55
Compare
optimisan
changed the base branch from
main
to
users/Akshat-Oke/12-09-spiller_deatach_legacy_pass_and_supply_analyses_instead
| case RegAllocEvictionAdvisorAnalysisLegacy::AdvisorMode::Default: | ||
| Provider.reset( | ||
| new DefaultEvictionAdvisorProvider(/*NotAsRequested=*/false, Ctx)); | ||
| break; |
init is in the constructor. The new MF analysis lazily intiailizes the provider. The (wrapped) provider is the analysis result.
optimisan
force-pushed
the
users/Akshat-Oke/11-22-_codegen_newpm_port_regallocevictionadvisor_analysis_to_npm
branch
from
72eff55 to
37f96a9
Compare
| Provider.reset( | ||
| new DefaultEvictionAdvisorProvider(/*NotAsRequested=*/true, Ctx)); | ||
| #endif | ||
| assert(Provider && "EvictionAdvisorProvider cannot be null"); |
There was a problem hiding this comment.
Pretty sure this is a dead assert. new doesn't return null
| case RegAllocEvictionAdvisorAnalysis::AdvisorMode::Default: | ||
| Ret = new DefaultEvictionAdvisorAnalysis(/*NotAsRequested*/ false); | ||
| case RegAllocEvictionAdvisorAnalysisLegacy::AdvisorMode::Default: | ||
| return new DefaultEvictionAdvisorAnalysisLegacy(/*NotAsRequested*/ false); |
There was a problem hiding this comment.
| return new DefaultEvictionAdvisorAnalysisLegacy(/*NotAsRequested*/ false); | |
| return new DefaultEvictionAdvisorAnalysisLegacy(/*NotAsRequested=*/ false); |
Legacy pass used to provide the advisor, so this extracts that logic into a provider class used by both analysis passes.
All three (Default, Release, Development) legacy passes
*AdvisorAnalysisare basically renamed to*AdvisorProvider, so the actual legacy wrapper passes are*AdvisorAnalysisLegacy.There is only one NPM analysis
RegAllocEvictionAnalysisthat switches between the three providers in the::runmethod, to be cached by the NPM.Also adds
RequireAnalysis<RegAllocEvictionAnalysis>to the optimized target reg alloc codegen builder.