[PATCH] D115707: [NFC][regalloc] Introduce the RegAllocEvictionAdvisorAnalysis
Mircea Trofin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 15 18:30:41 PST 2021
mtrofin added inline comments.
================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp:38-42
+static cl::opt<bool> EnableLocalReassignment(
+ "enable-local-reassign", cl::Hidden,
+ cl::desc("Local reassignment can yield better allocation decisions, but "
+ "may be compile time intensive"),
+ cl::init(false));
----------------
wenlei wrote:
> Can we keep this in greedy?
We can, and just pass it through. I can try that.
================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp:51-52
+namespace {
+class DefaultEvictionAdvisorAnalysis final
+ : public RegAllocEvictionAdvisorAnalysis {
+public:
----------------
wenlei wrote:
> Why do we need hierarchy for analysis? I would imagine that the inputs for different advisor are similar, in which case, would it be cleaner if we use one analysis that dispatches to different advisor based on Mode/Kind? That is what InlineAdvisorAnalysis does.
In the inliner case, the scope is module-wide, i.e. all the accounting and logging we do is per module. Here, advisors are per function, but we still need to have state shared by the ML advisors: the model evaluators (expensive to set up, cheap to use, and not tied to any particular function); and logs, in the training case, which are per-function, but we want to serialize them all once at doFinalization, to produce one file.
================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp:113-115
+ EnableLocalReassign(EnableLocalReassignment ||
+ MF.getSubtarget().enableRALocalReassignment(
+ MF.getTarget().getOptLevel())) {}
----------------
wenlei wrote:
> nit: move this piece below into a helper for use in both advisor and RAGreedy.
>
> ```
> EnableLocalReassignment ||
> MF.getSubtarget().enableRALocalReassignment(
> MF.getTarget().getOptLevel())
> ```
>
> This also reminds me of the advisor layer issue discussed in the previous patch. The advisor is tied to greedy. Would be good to address that soon.
I could frontend that, but I'm not sure it would help in this case: we don't appear to be using this field anywhere else in RAGreedy, it's only used by the methods that would move with the default advisor. (Maybe I'm missing something though?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115707/new/
https://reviews.llvm.org/D115707
More information about the llvm-commits
mailing list