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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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));
----------------
Can we keep this in greedy?


================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp:51-52
+namespace {
+class DefaultEvictionAdvisorAnalysis final
+    : public RegAllocEvictionAdvisorAnalysis {
+public:
----------------
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. 


================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp:89-90
+    return Ret;
+  dbgs() << "Requested regalloc eviction advisor analysis could be created. "
+            "Using default";
+  return new DefaultEvictionAdvisorAnalysis();
----------------
LLVM_DEBUG


================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp:113-115
+      EnableLocalReassign(EnableLocalReassignment ||
+                          MF.getSubtarget().enableRALocalReassignment(
+                              MF.getTarget().getOptLevel())) {}
----------------
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.


================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.h:238
+public:
+  enum class AdvisorKind : int { Default, Release, Development };
+
----------------
nit: Kind->Mode to be consistent with inline advisor?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115707/new/

https://reviews.llvm.org/D115707