This is an archive of the discontinued LLVM Phabricator instance.

[MLGO] Add per-instruction MBB frequencies to regalloc dev features
ClosedPublic

Authored by aidengrossman on Sep 19 2022, 12:56 AM.

Details

Summary

This commit adds in two new features to the ML regalloc eviction
analysis that can be used in ML models, a vector of MBB frequencies and
a vector of indicies mapping instructions to their corresponding basic
blocks. This will allow for further experimentation with per-instruction
features and give a lot more flexibility for future experimentation over
how we're extracting MBB frequency data currently.

Diff Detail

Event Timeline

aidengrossman created this revision.Sep 19 2022, 12:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2022, 12:56 AM
aidengrossman requested review of this revision.Sep 19 2022, 12:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2022, 12:56 AM

Here's the patch for the MBB based features. I've set it up in a way that isn't necessarily the most efficienct, but should allow for extra flexibility for experimentation on the ML side. I've opted to set up the main MBB feature extraction function to get called at every loop iteration in the current per-instruction feature extraction code, but due to how we currently have unit testing setup, this adds a couple more parameters to that function and leaves things a little bit less elegant than they probably could be. I think this should be fine for now, but I can definitely refactor to a different paradigm if necessary, but figured it would be better to discuss solutions first (and see if it's even necessary) before doing that work.

mtrofin added inline comments.Sep 19 2022, 11:08 AM
llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp
1057

Why not use the MBB address?

aidengrossman added inline comments.Sep 19 2022, 11:13 AM
llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp
1057

Give me a little bit of time to do some experimentation/investigation, but I tried calling getSectionIDNum() and utilizing that, but it would only return a single value for all the MBBs in the MF (only a single value would appear in the map).

mtrofin added inline comments.Sep 19 2022, 12:11 PM
llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp
1057

I mean &MBB. I.e. MachineBasicBlock*

Moved from mapping MBBs to indicies through the use of MBB pointers rather
than grabbing their names as strings.

aidengrossman added inline comments.Sep 24 2022, 12:11 AM
llvm/lib/CodeGen/MLRegallocEvictAdvisor.cpp
1057

Mapping using the MBB pointer as the key works perfectly and avoids passing around/generating strings. I've switched over. I had to do some slightly unorthodox casting to generate some test values in the unittest. I've added a comment explaining it, but if necessary I can modify it.

Refactored test pointer generation/arithmetic in MBB test case to avoid
compiler warning.

mtrofin added inline comments.Sep 26 2022, 9:55 AM
llvm/unittests/CodeGen/MLRegallocDevelopmentFeatures.cpp
263

Try rather making real MBB objects instead - the problem is that, if later we start dereferencing the MBB, this test will probably fail, and probably with non-deterministic failures - because the addressed memory will be invalid.

Should be like a 2-liner, make a Function, a MachineFunction, and create MBBs. I think some unittests have examples.

Switched from using dummy pointer values in development regalloc feature
test to using pointers to actual MBBs in case we ever end up dereferencing
these values at some point in the future.

mtrofin accepted this revision.Sep 27 2022, 4:34 PM
mtrofin added a reviewer: jacobhegna.

lgtm, and I apologize, but I should have added Jacob earlier to ptal, since he's also working on this. Please let him also take a look before landing.

This revision is now accepted and ready to land.Sep 27 2022, 4:36 PM

lgtm, this will be really useful!

just so I understand, mbb_frequencies will be of shape (1, 100) and if there are less than 100 mbbs, the rest of the entries will be 0? and mbb_mapping will of shape (1, 300) and contains the indices of the mbb for each instruction?

just a small question/nit: why use a one-hot encoding for the instruction opcode features but an index encoding for the frequencies?

just so I understand, mbb_frequencies will be of shape (1, 100) and if there are less than 100 mbbs, the rest of the entries will be 0? and mbb_mapping will of shape (1, 300) and contains the indices of the mbb for each instruction?

Yes. Up to 100 MBBs have their frequency data stored and otherwise everything is zero padded. The mapping vector is intended to work exactly as you describe. I settled on 100 MBBs after monitoring a LLVM compile after setting up some debug logging to see how many MBBs there were in each eviction problem and didn't see any with more than 100.

just a small question/nit: why use a one-hot encoding for the instruction opcode features but an index encoding for the frequencies?

I did this to give added flexibility in terms of how we integrate these features on the ML side. Currently just encoding a vector of length (1, ModelMaxSupportedInstructionCount) with the MBB frequencies would work for what I have been experimenting with, but if we want to do anything else in the future, the way this is setup gives added flexibility in that area (ie grouping instructions by MBB). This is the same reason I set up passing the instructions through an opcode vector and a binary mapping matrix rather than just passing the opcodes directly in a (33, 300) matrix.

jacobhegna accepted this revision.Sep 28 2022, 11:10 AM

sounds good!