asm: introduce a new x64 assembler #10110
Open
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.
abrown
commented
Jan 25, 2025
| @@ -1648,8 +1646,6 @@ block0(v0: i8x16, v1: i32): | |||
| ; addb %al, (%rax) | |||
| ; addb %al, (%rax) | |||
| ; addb %al, (%rax) | |||
| ; addb %al, (%rax) | |||
| ; addb %bh, %bh | |||
There was a problem hiding this comment.
Looking at this CLIF diffs, I see some potential problems: removed instructions, different immediates, potential add -> or miscompilations... This bears another look.
There was a problem hiding this comment.
(Pretty sure those add -> or "miscompilations" were just bits changing in the constant pool... not sure why they're changing, though).
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
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).