[PATCH] D133616: [MLGO] ML Regalloc Priority Advisor

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

Overall fine, I think you should still split out the change that introduces the `logRewardIfNeeded` as a NFC.

For the main change (i.e. this after rebasing with the above NFC), you also need test.



================
Comment at: llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp:424
 
+  virtual void logRewardIfNeeded(const MachineFunction &MF,
+                                 float Reward) override {
----------------
in the overridden case, drop the `virtual`.


================
Comment at: llvm/lib/CodeGen/MLRegallocPriorityAdvisor.cpp:121
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<SlotIndexes>();
+    RegAllocPriorityAdvisorAnalysis::getAnalysisUsage(AU);
----------------
`AU.setPreservesAll()`, too.


================
Comment at: llvm/lib/CodeGen/MLRegallocPriorityAdvisor.cpp:181
+  /// one. This is used to inject the score by the RegAllocScoring pass.
+  virtual Logger *getLogger(const MachineFunction &MF) const {
+    auto I = LogMap.find(MF.getName());
----------------
why is this virtual?


================
Comment at: llvm/lib/CodeGen/MLRegallocPriorityAdvisor.cpp:196
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<SlotIndexes>();
+    RegAllocPriorityAdvisorAnalysis::getAnalysisUsage(AU);
----------------
`AU.setPreservesAll()`


================
Comment at: llvm/lib/CodeGen/MLRegallocPriorityAdvisor.cpp:300
+  if (isa<ModelUnderTrainingRunner>(getRunner())) {
+    Prio = MLPriorityAdvisor::getPriority(LI);
+  } else {
----------------
There's a loss here due to the ping-ponging between float and unsigned and then back to float for logging. I imagine we want to stay in float domain for training, though. One way to avoid unnecessary lossiness is to: factor `getPriority` in the ML case into a `getPriorityImpl` that returns a float, and then `getPriority` just casts.

The oddity is that here you'd have to keep track of the "float" priorty as well as the unisgned one, so you'd have a `float TrainingPrio` and a `unsigned Prio`; or have Prio be a double and cast it to float when logging and to unsigned when returning, but you still need the `getPriorityImpl`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133616