Page MenuHomePhabricator

[NFC][MLGO] ML Regalloc Priority Advisor
ClosedPublic

Authored by Flpha0830 on Aug 4 2022, 4:33 PM.

Details

Summary

This patch introduces the priority analysis and the priority advisor,
the default implementation, and the scaffolding for introducing the
other implementations of the advisor.

Diff Detail

Event Timeline

Flpha0830 created this revision.Aug 4 2022, 4:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 4:33 PM
Flpha0830 requested review of this revision.Aug 4 2022, 4:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 4:33 PM
Flpha0830 updated this revision to Diff 450897.Aug 8 2022, 12:00 PM
This comment was removed by Flpha0830.

Add a patch description: what is this patch trying to achieve?

llvm/lib/CodeGen/MLRegallocPriorityAdvisor.cpp
1 ↗(On Diff #450897)

This should come in a different patch.

llvm/lib/CodeGen/RegAllocPriorityAdvisor.cpp
30

new line

llvm/lib/CodeGen/RegAllocPriorityAdvisor.h
26

I'd prefer this were introduced in a subsequent change. This current change can stay focused at factoring out the prioritization logic behind the RegAllocPriorityAdvisor interface.

59

add a new line

Flpha0830 updated this revision to Diff 450928.Aug 8 2022, 1:42 PM

This patch introduces the priority analysis and the priority advisor,
the default implementation, and the scaffolding for introducing the
other implementations of the advisor.

Add LICENSE and remove unused code

barannikov88 added inline comments.
llvm/lib/CodeGen/RegAllocPriorityAdvisor.h
12

Header guard is still missing

mtrofin added inline comments.Aug 10 2022, 8:00 AM
llvm/lib/CodeGen/RegAllocPriorityAdvisor.cpp
2

file header

llvm/lib/CodeGen/RegAllocPriorityAdvisor.h
2

add a header (see other .h files)

MatzeB added inline comments.Aug 10 2022, 9:27 AM
llvm/lib/CodeGen/RegAllocPriorityAdvisor.h
2

This comment seems off/wrong.

7–17

We really should have some comments here, either on the file header or the classes to have at least some sentences explaining what this is doing / used for.

Flpha0830 updated this revision to Diff 452296.Aug 12 2022, 1:47 PM
Flpha0830 marked 2 inline comments as done.

Add header guard and some comments

mtrofin added inline comments.Aug 12 2022, 3:00 PM
llvm/lib/CodeGen/RegAllocGreedy.cpp
357

rename to getPriority, and it should just take LIas parameter. Then, its implementation is all the stuff above.

llvm/lib/CodeGen/RegAllocPriorityAdvisor.h
35

remove logReward. this is something that only matters in the training scenario, so it doesn't belong on the abstraction.

38

do you need MF and RA here? IIUC, you just need the LiveInterval (passed in); maybe have the advisor take just RAGreedy, so it can get to ExtraInfo and LIS from it. At this stage, too, the ctor can be public.

45

remove the analysis stuff, it'll come in with a subsequent patch

83

remove logReward.

Flpha0830 updated this revision to Diff 452372.Aug 12 2022, 9:28 PM
Flpha0830 marked 3 inline comments as done.

Remove the analysis stuff and logReward.

Flpha0830 updated this revision to Diff 452379.Aug 12 2022, 9:56 PM
Flpha0830 marked 7 inline comments as done.

Remove MF

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

llvm/lib/CodeGen/CMakeLists.txt
171

nit: place it in alphabetical order, I think this goes under PBQP

llvm/lib/CodeGen/RegAllocGreedy.cpp
298

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.

llvm/lib/CodeGen/RegAllocGreedy.h
152

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

153

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.)

llvm/lib/CodeGen/RegAllocPriorityAdvisor.h
45
SlotIndexes * const Indexes;
const bool RegClassPriorityTrumpsGlobalness;

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

mtrofin added a comment.EditedAug 15 2022, 9:46 AM

This patch introduces the priority analysis and the priority advisor,
the default implementation, and the scaffolding for introducing the
other implementations of the advisor.

Please remember to:

  • add this to the git commit (i.e. git commit --amend and make sure this is the description)
  • also add this to the review message (from Edit Revision button on the right)
Flpha0830 updated this revision to Diff 452802.Aug 15 2022, 2:01 PM
Flpha0830 marked an inline comment as done.

Remove LiveRangeStage parameter and Pass LiveInterval by reference.

Flpha0830 edited the summary of this revision. (Show Details)Aug 15 2022, 2:02 PM
mtrofin accepted this revision.Aug 15 2022, 2:04 PM
This revision is now accepted and ready to land.Aug 15 2022, 2:04 PM
MatzeB added inline comments.Aug 15 2022, 6:30 PM
llvm/lib/CodeGen/RegAllocGreedy.cpp
357

Did you consider the precision loss that can happen when casting a 32bit unsigned number to 32bit float?

MatzeB added inline comments.Aug 15 2022, 6:35 PM
llvm/lib/CodeGen/RegAllocPriorityAdvisor.h
2

You marked this comment as "done", but this still reads "interference resolution" which I am pretty sure makes no sense.

Flpha0830 marked 5 inline comments as done and an inline comment as not done.

Leave the value as unsigned

mtrofin accepted this revision.Aug 17 2022, 8:32 AM
Flpha0830 marked 3 inline comments as done.Aug 17 2022, 10:21 AM
Flpha0830 added inline comments.
llvm/lib/CodeGen/RegAllocGreedy.cpp
357

Thanks for pointing this out. Because ML prefers small, bounded float values, we probably need float values. We can address this subsequently.

llvm/lib/CodeGen/RegAllocPriorityAdvisor.h
2

Thanks for the catch. This time it's fixed.

Flpha0830 updated this revision to Diff 453814.Aug 18 2022, 3:24 PM
Flpha0830 marked 2 inline comments as done.

Fix conflicts

This revision was landed with ongoing or failed builds.Aug 18 2022, 3:34 PM
This revision was automatically updated to reflect the committed changes.