[PATCH] D131220: [NFC][MLGO] ML Regalloc Priority Advisor

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

Nice! This is almost ready to go, left a few comments that should be easy to address.



================
Comment at: llvm/lib/CodeGen/CMakeLists.txt:174
   RegAllocEvictionAdvisor.cpp
+  RegAllocPriorityAdvisor.cpp
   RegAllocFast.cpp
----------------
nit: place it in alphabetical order, I think this goes under PBQP


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:303
+
+float DefaultPriorityAdvisor::getPriority(const LiveInterval *LI, LiveRangeStage Stage) const {
+  const unsigned Size = LI->getSize();
----------------
make it `const LiveInterval &LI`: it's never expected to be nullptr, so pass-by-reference strongly suggests that to one reading the code.

also: remove the LiveRangeStage parameter. You can get it from `RA.getExtraInfo().getStage(LI)`. This makes the interface very simple: you pass in a LI, you get a priority, the rest of the values that may factor in the calculation are up to the advisor implementation to get from wherever - other analyses, or the RA.


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.h:154
+  // Interface to priority advisers
+  bool getRegClassPriorityTrumpsGlobalness() const { return RegClassPriorityTrumpsGlobalness; }
+  SlotIndexes *getIndexes() const { return Indexes; }
----------------
this API almost feels like could be removed and we just pass `RegClassPriorityTrumpsGlobalness` via the ctor to the advisor, but we can easily do that once the full design is in, so lgtm


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.h:155
+  bool getRegClassPriorityTrumpsGlobalness() const { return RegClassPriorityTrumpsGlobalness; }
+  SlotIndexes *getIndexes() const { return Indexes; }
+  // end (interface to priority advisers)
----------------
you can put a FIXME comment above to say this can be removed once we have an analysis, because the analysis can getAnalysis<SlotIndexes> and pass this. See elsewhere in the codebase how these comments look like (basically `//FIXME: this is unnecessary once priority advisers are created by an analysis pass, which can fetch the SlotIndexes analysis itself.`)


================
Comment at: llvm/lib/CodeGen/RegAllocPriorityAdvisor.h:44
+  const RegisterClassInfo &RegClassInfo;
+  SlotIndexes *Indexes;
+  bool RegClassPriorityTrumpsGlobalness;
----------------
```
SlotIndexes * const Indexes;
const bool RegClassPriorityTrumpsGlobalness;
```

because they are ctor-time init-ed and then never change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131220