This is an archive of the discontinued LLVM Phabricator instance.

[MLGO] ML Regalloc Priority Advisor
ClosedPublic

Authored by Flpha0830 on Sep 9 2022, 3:17 PM.

Details

Summary

The bulk of the implementation is common between 'release' mode (==AOT-ed
model) and 'development' mode (for training), the main difference is
that in development mode, we may also log features (for training logs),
inject scoring information and then produce the log file.

Diff Detail

Event Timeline

Flpha0830 created this revision.Sep 9 2022, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 3:17 PM
Flpha0830 requested review of this revision.Sep 9 2022, 3:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 3:17 PM

I think we need to separate in a first patch the changes needed to reuse regallocscoring. For those, I think maybe a simple solution right now would be to have the base RegAllocEvictionAdvisorAnalysis and RegAllocPriorityAdvisorAnalysis (the base types) have a virtual void logRewardIfNeeded(float Reward) method. By default it does nothing. Just the Development variant does something. So that way you don't need the additional .h for the scoring, just moving it to a .cpp would be sufficient.

Add logRewardIfNeeded method

Add a model test

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.

llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp
424 ↗(On Diff #461154)

in the overridden case, drop the virtual.

llvm/lib/CodeGen/MLRegallocPriorityAdvisor.cpp
122

AU.setPreservesAll(), too.

182

why is this virtual?

197

AU.setPreservesAll()

301

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.

Flpha0830 updated this revision to Diff 463678.Sep 28 2022, 2:52 PM

add a getPriorityImpl and update the model test

Flpha0830 marked 4 inline comments as done.Sep 28 2022, 2:56 PM
mtrofin accepted this revision.Sep 29 2022, 8:31 AM

added Yundi for the test model generator, please wait for her feedback before submitting.

On my side, lgtm, left some comments about not trying to enable release mode at this point.

(self-note: there's some boilerplate we can further simplify, I think like ctoring of the dev mode could be more streamlined - I'll take a look into that)

llvm/lib/CodeGen/RegAllocPriorityAdvisor.cpp
39

I'd remove this. We don't currently support constructing the release model - that also needs some stuff in CMakeLists.txt. All good, all we need is the development stuff anyway, and all the release stuff is common anyway.

84–86

check LLVM_HAVE_TF_AOT_REGALLOCPRIORITYMODEL here (which nothing sets, which is good right now). We need to do something similar for evictadvisor now, too (not in this patch)

This revision is now accepted and ready to land.Sep 29 2022, 8:31 AM
yundiqian accepted this revision.Sep 29 2022, 10:31 AM
Flpha0830 updated this revision to Diff 464034.Sep 29 2022, 2:35 PM

use LLVM_HAVE_TF_AOT_REGALLOCPRIORITYMODEL

This revision was landed with ongoing or failed builds.Sep 29 2022, 2:56 PM
This revision was automatically updated to reflect the committed changes.