[PATCH] D115707: [NFC][regalloc] Introduce the RegAllocEvictionAdvisorAnalysis

[Mircea Trofin via Phabricator via llvm-commits]

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