[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:51-52
+namespace {
+class DefaultEvictionAdvisorAnalysis final
+    : public RegAllocEvictionAdvisorAnalysis {
+public:
----------------
mtrofin wrote:
> 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.
> 
Okay, so IIUC, the states that need to be shared by advisors are specific to different Modes? Otherwise, we could still have a generic analysis. 

I'm asking because by looking at the default analysis it doesn't seem necessary, but it might make sense if it's needed for the actual ML advisors. 


================
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();
----------------
mtrofin wrote:
> wenlei wrote:
> > LLVM_DEBUG
> I think we want to warn back to the user. Maybe we should instead mark somehow the created object (which can only be the Default anyway) and in its doInitialization, where we have a LLVMContext, emit an error? (not sure if there's a precedent)
> 
> alternatively: we can have the analysis as a final class and delegate to an implementation which we create in doInitialization. The implementation then can have additional requirements for doInitialization/doFinalization and getAnalysisUsage, if it needs.
> 
> wdyt?
> I think we want to warn back to the user. Maybe we should instead mark somehow the created object (which can only be the Default anyway) and in its doInitialization, where we have a LLVMContext, emit an error? (not sure if there's a precedent) 

It feels to me that this is just an intermediate state. The switch uses enum so it has to be one of the three values, then when all modes are implemented, such error path should never happen. If it's just to guard against misuse in this intermediate state, I guess an assertion is fine too as it's temporary anyways? 



================
Comment at: llvm/lib/CodeGen/RegAllocEvictionAdvisor.cpp:113-115
+      EnableLocalReassign(EnableLocalReassignment ||
+                          MF.getSubtarget().enableRALocalReassignment(
+                              MF.getTarget().getOptLevel())) {}
----------------
mtrofin wrote:
> 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?)
Took another look, you're right. sorry for the confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115707