Cranelift: Add a new backend for emitting Pulley bytecode #9089
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fitzgen
force-pushed
the
pulley-cranelift-backend
branch
from
b0812e7
to
ab8f1b8
Compare
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
alexcrichton
approved these changes
Aug 12, 2024
Comment on lines
+468
to
+398
| if let Ok(offset) = i8::try_from(offset) { | ||
| enc::xconst8(sink, dst, offset); | ||
| } else if let Ok(offset) = i16::try_from(offset) { | ||
| enc::xconst16(sink, dst, offset); | ||
| } else if let Ok(offset) = i32::try_from(offset) { | ||
| enc::xconst32(sink, dst, offset); | ||
| } else { | ||
| enc::xconst64(sink, dst, offset); | ||
| } |
There was a problem hiding this comment.
I think I saw this as well in abi.rs as well as lower.isle perhaps, coul the Rust-side parts be deduplicated into a helper?
There was a problem hiding this comment.
The one in abi.rs is constructing MInst::Xconst* variants while this one is calling differentenc::xconst* functions, so there isn't a great way to dedupe this. We could make something that is abstract enough to handle both but that abstraction won't actually be any more concise, simpler, or easy to understand, unfortunately.
fitzgen
force-pushed
the
pulley-cranelift-backend
branch
from
ee57c2b
to
77b1865
Compare
This commit adds two new backends for Cranelift that emits 32- and 64-bit Pulley bytecode. The backends are both actually the same, with a common implementation living in `cranelift/codegen/src/isa/pulley_shared`. Each backend configures an ISA flag that determines the pointer size, and lowering inspects this flag's value when lowering memory accesses. To avoid multiple ISLE compilation units, and to avoid compiling duplicate copies of Pulley's generated `MInst`, I couldn't use `MInst` as the `MachInst` implementation directly. Instead, there is an `InstAndKind` type that is a newtype over the generated `MInst` but which also carries a phantom type parameter that implements the `PulleyTargetKind` trait. There are two implementations of this trait, a 32- and 64-bit version. This is necessary because there are various static trait methods for the mach backend which we must implement, and which return the pointer width, but don't have access to any `self`. Therefore, we are forced to monomorphize some amount of code. This type parameter is fairly infectious, and all the "big" backend types (`PulleyBackend<P>`, `PulleyABICallSite<P>`, etc...) are parameterized over it. Nonetheless, not everything is parameterized over a `PulleyTargetKind`, and we manage to avoid duplicate `MInst` definitions and lowering code. Note that many methods are still stubbed out with `todo!`s. It is expected that we will fill in those implementations as the work on Pulley progresses.
fitzgen
force-pushed
the
pulley-cranelift-backend
branch
from
77b1865
to
ce5c44d
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds two new backends for Cranelift that emits 32- and 64-bit Pulley
bytecode. The backends are both actually the same, with a common implementation
living in
cranelift/codegen/src/isa/pulley_shared. Each backend configures anISA flag that determines the pointer size, and lowering inspects this flag's
value when lowering memory accesses.
To avoid multiple ISLE compilation units, and to avoid compiling duplicate
copies of Pulley's generated
MInst, I couldn't useMInstas theMachInstimplementation directly. Instead, there is an
InstAndKindtype that is anewtype over the generated
MInstbut which also carries a phantom typeparameter that implements the
PulleyTargetKindtrait. There are twoimplementations of this trait, a 32- and 64-bit version. This is necessary
because there are various static trait methods for the mach backend which we
must implement, and which return the pointer width, but don't have access to any
self. Therefore, we are forced to monomorphize some amount of code. This typeparameter is fairly infectious, and all the "big" backend
types (
PulleyBackend<P>,PulleyABICallSite<P>, etc...) are parameterizedover it. Nonetheless, not everything is parameterized over a
PulleyTargetKind,and we manage to avoid duplicate
MInstdefinitions and lowering code.Note that many methods are still stubbed out with
todo!s. It is expected thatwe will fill in those implementations as the work on Pulley progresses.
The first commit is #9085; this depends on that PR.