asm: introduce a new x64 assembler #10110
Merged
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
abrown
force-pushed
the
assembler-upstream
branch
2 times, most recently
from
d209114 to
9140ca2
Compare
github-actions
bot
added
the
cranelift
abrown
force-pushed
the
assembler-upstream
branch
2 times, most recently
from
943fb8e to
d694e80
Compare
abrown
commented
Jan 25, 2025
Comment on lines
4698
to
4705
| // These values are transcribed from is happening in | ||
| // `SyntheticAmode::finalize`. This, plus the `Into` logic converting a | ||
| // `SyntheticAmode` to its external counterpart, are | ||
| let frame = state.frame_layout(); | ||
| known_offsets[external::offsets::KEY_INCOMING_ARG] = | ||
| i32::try_from(frame.tail_args_size + frame.setup_area_size).unwrap(); | ||
| known_offsets[external::offsets::KEY_SLOT_OFFSET] = | ||
| i32::try_from(frame.outgoing_args_size).unwrap(); |
There was a problem hiding this comment.
@cfallin, I'm not a big fan of this KnownOffsetTable approach: it seems like we may want to just add appropriate CodeSink/MachBuffer methods to propagate this kind of thing.
There was a problem hiding this comment.
Perhaps! Were you thinking we'd have specific trait methods for e.g. incoming_arg and slot_offset? Or that we'd still be generic over the set of known offsets but have a query method?
I'm totally fine with the latter, in fact it's probably even cleaner; for the former I'd be concerned about baking details of Cranelift's ABI impl into the lower-level library and would still want to follow the analogy of a real assembler letting one define symbolic constants and use them with specific values plugged in. Happy to look at proposed diff if you have one.
There was a problem hiding this comment.
I agree, we should avoid Cranelift-izing this assembler as much as possible. I guess things I don't like about the known offset table approach is that (a) we rebuild this table for every emissions and (b) building it the way I'm doing it here feels quite fragile. How long before things get out of sync with the keys used during amode conversion?!
abrown
commented
Jan 25, 2025
abrown
force-pushed
the
assembler-upstream
branch
from
d694e80 to
574176f
Compare
This change adds some initial logic implementing an external assembler for Cranelift's x64 backend, as proposed in RFC [bytecodealliance#41]. This adds two crates: - the `cranelift/assembler/meta` crate defines the instructions; to print out the defined instructions use `cargo run -p cranelift-assembler-meta` - the `cranelift/assembler` crate exposes the generated Rust code for those instructions; to see the path to the generated code use `cargo run -p cranelift-assembler` The assembler itself is straight-forward enough (modulo the code generation, of course); its integration into `cranelift-codegen` is what is most tricky about this change. Instructions that we will emit in the new assembler are contained in the `Inst::External` variant. This unfortunately increases the memory size of `Inst`, but only temporarily if we end up removing the extra `enum` indirection by adopting the new assembler wholesale. Another integration point is ISLE: we generate ISLE definitions and a Rust helper macro to make the external assembler instructions accessible to ISLE lowering. This change introduces some duplication: the encoding logic (e.g. for REX instructions) currently lives both in `cranelift-codegen` and the new assembler crate. The `Formatter` logic for the assembler `meta` crate is quite similar to the other `meta` crate. This minimal duplication felt worth the additional safety provided by the new assembler. The `cranelift-assembler` crate is fuzzable (see the `README.md`). It will generate instructions with randomized operands and compare their encoding and pretty-printed string to a known-good disassembler, currently `capstone`. This gives us confidence we previously didn't have regarding emission. In the future, we may want to think through how to fuzz (or otherwise check) the integration between `cranelift-codegen` and this new assembler level. [bytecodealliance#41]: bytecodealliance/rfcs#41
Using the new assembler's pretty-printing results in slightly different disassembly of compiled CLIF. This is because the assembler matches a certain configuration of `capstone`, causing the following obvious differences: - instructions with only two operands only print two operands; the original `MInst` instructions separate out the read-write operand into two separate operands (SSA-like) - the original instructions have some space padding after the instruction mnemonic, those from the new assembler do not This change uses the slightly new style as-is, but this is open for debate; we can change the configuration of `capstone` that we fuzz against. My only preferences would be to (1) retain some way to visually distinguish the new assembler instructions in the disassembly (temporarily, for debugging) and (2) eventually transition to pretty-printing instructions in Intel-style (`rw, r`) instead of the current (`r, rw`).
Though it is likely that `rustfmt` is present in a Rust environment, some CI tasks do not have this tool installed. To handle this case (plus the chance that other Wasmtime builds are similar), this change skips formatting with a `stderr` warning when `rustfmt` fails.
In order to satisfy `ci/publish.rs`, it would appear that we need to use a version that matches the rest of the Cranelift crates.
abrown
force-pushed
the
assembler-upstream
branch
from
574176f to
f6e4f1d
Compare
cfallin
approved these changes
Jan 29, 2025
There was a problem hiding this comment.
This looks really really good -- thanks for spawning this idea and working to build it out!
Most of my feedback you already got offline as this was being built and the general shape is therefore what I'd expect and am overall quite happy with. A bunch of nits below but nothing major -- let's get this merged and we can iterate and start the migration process.
Thanks again!
Comment on lines
4698
to
4705
| // These values are transcribed from is happening in | ||
| // `SyntheticAmode::finalize`. This, plus the `Into` logic converting a | ||
| // `SyntheticAmode` to its external counterpart, are | ||
| let frame = state.frame_layout(); | ||
| known_offsets[external::offsets::KEY_INCOMING_ARG] = | ||
| i32::try_from(frame.tail_args_size + frame.setup_area_size).unwrap(); | ||
| known_offsets[external::offsets::KEY_SLOT_OFFSET] = | ||
| i32::try_from(frame.outgoing_args_size).unwrap(); |
There was a problem hiding this comment.
Perhaps! Were you thinking we'd have specific trait methods for e.g. incoming_arg and slot_offset? Or that we'd still be generic over the set of known offsets but have a query method?
I'm totally fine with the latter, in fact it's probably even cleaner; for the former I'd be concerned about baking details of Cranelift's ABI impl into the lower-level library and would still want to follow the analogy of a real assembler letting one define symbolic constants and use them with specific values plugged in. Happy to look at proposed diff if you have one.
|
(feel free to ignore my comments and respond after landing) |
|
Oh one blocking thing if that's ok: this adds a hard dependency of cranelift on capstone which I believe wasn't previously there. Would it be possible to move that to a dev-dependency or a feature'd dependency? |
|
(I sent a PR for that change as abrown#10) |
* Fuzzing updates for cranelift-assembler-x64 * Ensure fuzzers build on CI * Move fuzz crate into the main workspace * Move `fuzz.rs` support code directly into fuzzer * Move `capstone` dependency into the fuzzer * Make `arbitrary` an optional dependency Shuffle around a few things in a few locations for this.
Co-authored-by: Alex Crichton <[email protected]>
Cranelift's existing lowering for 8-bit and 16-bit reg-reg `AND` used the wider version of the instruction--the 32-bit reg-reg `AND`. As pointed out by @cfallin [here], this was likely due to avoid partial register stalls. This change keeps that lowering by distinguishing more precisely between `GprMemImm` that are in register or memory. [here]: bytecodealliance#10110 (comment)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Apparently `rustfmt` is not found on the `x86_64-unknown-illumos` build. This change skips the action in this new case. prtest:full
This fixes errors with the `publish.rs` script. prtest:full
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
This latest CI failure is under QEMU-emulated s390x. Have we ever observed a segfault here before? I can't imagine this has much to do with the new |
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Feb 4, 2025
In bytecodealliance#10110, I originally intended to use `arbitrary` implementations in two ways: for long-running fuzz testing (e.g., with OSS-Fuzz) but also for quick property testing with `cargo test`. This latter use case could replace the tedious emit tests we had to write in `cranelift-codegen` _and_ find corner cases that we otherwise might not explore. It helped me during development: just run `cargo test` to check if anything is obviously wrong. `arbtest` seemed to be able to run ~1000 test cases and found mistakes well within the one second time limit I gave it. @alexcrichton improved bytecodealliance#10110 by avoiding `Arbitrary` implementations everywhere and unconditionally depending on the `arbitrary` crate. This was the right change, but it removed the ability to property test using `cargo test`. What this change does is retain the general intent of his change (no extra dependencies) but add `Arbitrary` implementations for `cfg(test)` as well to run property tests during `cargo test`. The only downside I see here is the added complexity when conditionally compiling the fuzz-related bits: `#[cfg(any(test, feature = "fuzz"))]`. Perhaps there is a better way to do this, but this seemed to work fine. Let me know what you think.
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Feb 4, 2025
In bytecodealliance#10110, I originally intended to use `arbitrary` implementations in two ways: for long-running fuzz testing (e.g., with OSS-Fuzz) but also for quick property testing with `cargo test`. This latter use case could replace the tedious emit tests we had to write in `cranelift-codegen` _and_ find corner cases that we otherwise might not explore. It helped me during development: just run `cargo test` to check if anything is obviously wrong. `arbtest` seemed to be able to run ~1000 test cases and found mistakes well within the one second time limit I gave it. @alexcrichton improved bytecodealliance#10110 by avoiding `Arbitrary` implementations everywhere and unconditionally depending on the `arbitrary` crate. This was the right change, but it removed the ability to property test using `cargo test`. What this change does is retain the general intent of his change (no extra dependencies) but add `Arbitrary` implementations for `cfg(test)` as well to run property tests during `cargo test`. The only downside I see here is the added complexity when conditionally compiling the fuzz-related bits: `#[cfg(any(test, feature = "fuzz"))]`. Perhaps there is a better way to do this, but this seemed to work fine. Let me know what you think.
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Feb 4, 2025
In bytecodealliance#10110, I originally intended to use `arbitrary` implementations in two ways: for long-running fuzz testing (e.g., with OSS-Fuzz) but also for quick property testing with `cargo test`. This latter use case could replace the tedious emit tests we had to write in `cranelift-codegen` _and_ find corner cases that we otherwise might not explore. It helped me during development: just run `cargo test` to check if anything is obviously wrong. `arbtest` seemed to be able to run ~1000 test cases and found mistakes well within the one second time limit I gave it. @alexcrichton improved bytecodealliance#10110 by avoiding `Arbitrary` implementations everywhere and unconditionally depending on the `arbitrary` crate. This was the right change, but it removed the ability to property test using `cargo test`. What this change does is retain the general intent of his change (no extra dependencies) but add `Arbitrary` implementations for `cfg(test)` as well to run property tests during `cargo test`. The only downside I see here is the added complexity when conditionally compiling the fuzz-related bits: `#[cfg(any(test, feature = "fuzz"))]`. Perhaps there is a better way to do this, but this seemed to work fine. Let me know what you think.
abrown
added a commit
to abrown/wasmtime
that referenced
this pull request
Feb 4, 2025
In bytecodealliance#10110, I originally intended to use `arbitrary` implementations in two ways: for long-running fuzz testing (e.g., with OSS-Fuzz) but also for quick property testing with `cargo test`. This latter use case could replace the tedious emit tests we had to write in `cranelift-codegen` _and_ find corner cases that we otherwise might not explore. It helped me during development: just run `cargo test` to check if anything is obviously wrong. `arbtest` seemed to be able to run ~1000 test cases and found mistakes well within the one second time limit I gave it. @alexcrichton improved bytecodealliance#10110 by avoiding `Arbitrary` implementations everywhere and unconditionally depending on the `arbitrary` crate. This was the right change, but it removed the ability to property test using `cargo test`. What this change does is retain the general intent of his change (no extra dependencies) but add `Arbitrary` implementations for `cfg(test)` as well to run property tests during `cargo test`. The only downside I see here is the added complexity when conditionally compiling the fuzz-related bits: `#[cfg(any(test, feature = "fuzz"))]`. Perhaps there is a better way to do this, but this seemed to work fine. Let me know what you think.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 5, 2025
In #10110, I originally intended to use `arbitrary` implementations in two ways: for long-running fuzz testing (e.g., with OSS-Fuzz) but also for quick property testing with `cargo test`. This latter use case could replace the tedious emit tests we had to write in `cranelift-codegen` _and_ find corner cases that we otherwise might not explore. It helped me during development: just run `cargo test` to check if anything is obviously wrong. `arbtest` seemed to be able to run ~1000 test cases and found mistakes well within the one second time limit I gave it. @alexcrichton improved #10110 by avoiding `Arbitrary` implementations everywhere and unconditionally depending on the `arbitrary` crate. This was the right change, but it removed the ability to property test using `cargo test`. What this change does is retain the general intent of his change (no extra dependencies) but add `Arbitrary` implementations for `cfg(test)` as well to run property tests during `cargo test`. The only downside I see here is the added complexity when conditionally compiling the fuzz-related bits: `#[cfg(any(test, feature = "fuzz"))]`. Perhaps there is a better way to do this, but this seemed to work fine. Let me know what you think.
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 is a first step to providing an external assembler for
cranelift-codegenas described in #41. Each commit has further details, but the summary is that this direction would eventually move all assembler logic out into crates designed for easier checking (e.g., fuzzing).