Add support for using regalloc2 as a backend register allocation algorithm.#127
Add support for using regalloc2 as a backend register allocation algorithm.#127cfallin wants to merge 7 commits into
Conversation
…rithm. This PR adds a shim that translates regalloc.rs' view of the input program into a form that regalloc2 can use, and translates the allocations back to regalloc.rs-like results. This was co-developed and -optimized with regalloc2, as a sort of short-to-medium-term solution to the Cranelift use-case before Cranelift can be migrated to use the regalloc2 API directly. Performance-testing results show that this is a marginal improvement over existing (current default) BTRA, including the shim overhead, so I believe that this is a reasonable thing to use as a stepping-stone rather than wait for the above transition. It also includes the fuzz target `regalloc2` that runs the regalloc.rs checker with the new algorithm; I have fuzzed the combination of the two with this target, in addition to fuzzing just regalloc2 with its own fuzz targets.
| to_reg: Writable<RealReg>, | ||
| for_reg: Reg, | ||
| }, | ||
| /// As for DefReg, for for spillslots. |
|
Thanks for doing this work, and sorry I couldn't get to review it earlier. I will give it a look, probably in several phases, across next week. |
|
No rush, and sorry for throwing such a large PR at you! Thanks in advance for whatever time you're able to spend on it :-) |
|
Also the test failures seem to be new since I rebased to the latest regalloc2 -- I'll take a look at those next week and get this into a clean/passing state. |
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
A first batch of comments: I've looked at everything but the main thing in regalloc2.rs. Other changes look great so far!
| @@ -1,5 +1,7 @@ | |||
| //! Data structures for the whole crate. | |||
|
|
|||
| #![allow(dead_code)] | |||
There was a problem hiding this comment.
It's a bit unfortunate to regress on dead code, I am happy to keep it if it's intended to be for the short term and you have thoughts about removing it later, but out of curiosity, why was it needed? (Maybe it is easy to remove)
There was a problem hiding this comment.
Oddly enough, I went down a bit of a rabbithole trying to remove this, without success: if I remove the whole function I get a compile error, and if I remove the directive then I see a warning about dead code. Possibly something weird to do with alternative build configurations/features, but I wasn't able to work it out. I added a comment at least to say "it's definitely used" :-)
|
Thanks @bnjbvr and sorry for the delay in responding/updating here. I think I've addressed everything and I'm happy to address whatever other comments you may have! |
|
(And just to state for maximal clarity, the |
|
Adding @julian-seward1 as a reviewer as well after checking with him -- thanks! |
|
Closing old PR as the new plan is to use regalloc2 directly in Cranelift and so we won't need this compatibility shim anymore (thanks to all reviewers here though!). |
|
Thanks for the heads-up! I've looked in the wasmtime's repository, and both searching for "regalloc2" in issues and PRs doesn't show any relevant discussion. There's no open RFC to talk about it either (although I expect it is the best obvious path and might not even mandate a RFC). I've attended the last few Cranelift meetings too and there was no discussions about this, unless I missed something. Has this plan be discussed in some public forum that I missed? |
|
Making the move to regalloc2 has been one of the motivators for introducing ISLE: https://github.com/bytecodealliance/rfcs/pull/13/files#diff-eaec9e420ce24444f724c423e3c78a93525054404ba8c2ac399357f46949315fR172-R186 The transition to ISLE is not yet finished. I believe the plan is to wait on this before switching to regalloc2. |
|
Hey @bnjbvr -- sorry for not linking to more detail here! This plan (to use regalloc2 directly) is part of the Cranelift 2022 roadmap (see this section) which merged two weeks ago. And indeed it was part of the motivation for ISLE (more easily allowing cross-cutting changes to be made), as @bjorn3 links above. The timing for closing this now just had to do with me making another pass at cleaning up my old open PRs, no recent discussions or anything like that :-) |
|
Ah ok, thanks! Sorry, I mistakenly thought there was something else that I had missed. |
4 participants
This PR adds a shim that translates regalloc.rs' view of the input program into a form that regalloc2 can use, and translates the allocations back to regalloc.rs-like results.
This was co-developed and -optimized with regalloc2, as a sort of short-to-medium-term solution to the Cranelift use-case before Cranelift can be migrated to use the regalloc2 API directly. Performance-testing results show that this is a marginal improvement over existing (current default) BTRA, including the shim overhead, so I believe that this is a reasonable thing to use as a stepping-stone rather than wait for the above transition.
It also includes the fuzz target
regalloc2that runs the regalloc.rs checker with the new algorithm; I have fuzzed the combination of the two with this target, in addition to fuzzing just regalloc2 with its own fuzz targets.regalloc2is currently pending a relicensing (bytecodealliance/regalloc2#7) and I plan to publish 0.0.1 to crates.io once that is done. Before then, we cannot actually merge this PR, but I am hoping that this will at least allow us to parallelize the review effort :-)I've squashed the 53-commit development history into one commit for this PR; the original is on the
regalloc2-dev-historybranch in my fork.