Wasmtime GitHub notifications bot (Dec 08 2024 at 00:55): Wasmtime GitHub notifications bot (Dec 08 2024 at 00:55):alexcrichton opened PR #9765 from alexcrichton:pulley-new-instructions to bytecodealliance:main:
In helping others contribute to Pulley I've written up the steps I took
to make this commit itself. I've picked a random test inmisc_testsuite
that previously didn't work on Pulley and was relatively small. I
implemented a few lowerings and some new instructions and now the test
passes. I've addedpulley/CONTRIBUTING.mdto document the experience
along the way in case anyone else is interested to help out in the
future.
Wasmtime GitHub notifications bot (Dec 08 2024 at 00:55):alexcrichton requested fitzgen for a review on PR #9765.
Wasmtime GitHub notifications bot (Dec 08 2024 at 00:55):alexcrichton requested wasmtime-core-reviewers for a review on PR #9765.
Wasmtime GitHub notifications bot (Dec 08 2024 at 00:55):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #9765.
Wasmtime GitHub notifications bot (Dec 08 2024 at 00:55):alexcrichton requested wasmtime-default-reviewers for a review on PR #9765.
Wasmtime GitHub notifications bot (Dec 08 2024 at 00:56):alexcrichton commented on PR #9765:
I'll note that this is built on https://github.com/bytecodealliance/wasmtime/pull/9760 so only the last commit here is the guide part.
Wasmtime GitHub notifications bot (Dec 09 2024 at 15:39):alexcrichton updated PR #9765.
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
you're interested in helping to improve the situation. This is intended to be a
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
Just to make it more clear which args are for
cargoand which are forwasmtime:$ cargo run --features pulley -- wast --target pulley64 ./tests/misc_testsuite/control-flow.wast ```` ~~~
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen submitted PR review:
Looks great!
I have a bunch of nitpicks and suggestions to make things flow a little smoother IMO or to link to other references and resources, but those are tiny nitpicks and this is fantastic.
Excited to start onboarding new contributors with this!
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
## Adding an instruction to Pulley
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
`cranelift/codegen/src/isa/pulley_shared/lower.isle`. Our ISLE lowering rules are generally written like this: \```lisp (rule (lower <CLIF>) <Pulley>) \``` This means "when lowering Cranelift's mid-level IR down to Pulley bytecode, and we match the snippet `<CLIF>`, replace it with `<Pulley>`". (For more details, see the [ISLE Language Reference](https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/isle/docs/language-reference.md) and [How ISLE is Integrated with Cranelift](https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/docs/isle-integration.md).) In our case, we need to match an `sdiv` CLIF instruction that is operating on 32-bit values (this is the `(has_type $I32 ...)` bit) and then replace it with our new Pulley `xdiv32_s` instruction:
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
the same test under a variety of configurations, but the test is expected to fail under Pulley. You can update the `WastTest::should_fail` method in `crates/wast-util/src/lib.rs` to say the test is expected to pass, and then you
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
stray empty line
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
method is used to specifically match the width and signedness of the instruction itself, signed
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
Note that ISLE constructors for Pulley instructions, including the `pulley_xdivs32_s` constructor for our new `xdiv32_s` Pulley instruction, are automatically generated from the `for_each_op!` macro.
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
This defines the snake-case name of the instruction (`xdiv32_s`) that is used by the disassembler and visitor trait, the upper-camel-case name of the instruction (`I32DivS`) used for the Rust type and `enum` variant, and immediates and operands in the instruction itself. In this case it's a binary operation using integer ("x") registers. Note: By convention, we tend to include the class ("x") and width ("32") of registers operated upon by an instruction in its name. This distinguishes between, for example, 32-bit integer addition (`xadd32`) and 64-bit floating point addition (`fadd64`).
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
for an up-to-date list check out the `WastTest::should_fail` method in `crates/wast-util/src/lib.rs`. Here we're going
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
Success! But we aren't quite done yet: the new lowerings we added might have made additional tests that previously failed start passing as well. If so, then we also want to mark those tests as expected to pass now. We can double check by running the full `wast` test suite:
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
| ---------------------------------------------------------------------------- `xdiv32_s` from trait
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
FWIW, I would probably name the instruction
xdivs32but it really doesn't matter. What does matter is that the guide is consistent, so as not to confuse contributors:error[E0046]: not all trait items implemented, missing: `xdiv32_s` --> pulley/src/interp.rs:807:1 | 807 | impl OpVisitor for Interpreter<'_> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `xdiv32_s` in implementation
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
assertion is no longer true! Update the `WastTest::should_fail` method in `crates/wast-util/src/lib.rs` so that it expects the test to pass and we'll see:
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:19):fitzgen created PR review comment:
Could we make
done_trapgeneric over an instruction and then avoid the need to dolet me = self.current_pc::<...>();and instead doControlFlow::Break(self.done_trap::<...>())or even fold the
ControlFlow::Breakinto the helper?This both delays the computation until its demanded (which LLVM can probably do, but might be foiled if there are zero/overflow whatever checks hidden in there) and is a little more concise at usage/call sites.
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:20):fitzgen created PR review comment:
When we run our test again we don't get a compilation error anymore, but instead we see the ISLE lowering error again! \`\`\` $ cargo run --features pulley wast --target pulley64 ./tests/misc_testsuite/control-flow.wast Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.08s Running `target/debug/wasmtime wast --target pulley64 ./tests/misc_testsuite/control-flow.wast` Error: failed to run script file './tests/misc_testsuite/control-flow.wast' Caused by: 0: failed directive on ./tests/misc_testsuite/control-flow.wast:77:1 1: Compilation error: Unsupported feature: should be implemented in ISLE: inst = `v5 = sdiv.i32 v2, v3`, type = `Some(types::I32)` \`\`\` This leads us to the next part...
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:40):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:40):alexcrichton created PR review comment:
Heh you can see I switched halfway through :)
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:51):alexcrichton updated PR #9765.
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:52):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:52):alexcrichton created PR review comment:
Excellent idea :+1:
Wasmtime GitHub notifications bot (Dec 10 2024 at 15:52):alexcrichton requested fitzgen for a review on PR #9765.
Wasmtime GitHub notifications bot (Dec 10 2024 at 16:18):fitzgen created PR review comment:
Maybe also link to this PR at the end?
To view the complete pull request that implemented `xdiv32_s` and `xand32` Pulley instructions and got `./tests/misc_testsuite/control-flow.wast` passing (and also introduced this documentation) check out https://github.com/bytecodealliance/wasmtime/pull/9765 Thanks for helping out!
Wasmtime GitHub notifications bot (Dec 10 2024 at 16:18):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Dec 10 2024 at 16:23):alexcrichton updated PR #9765.
Wasmtime GitHub notifications bot (Dec 10 2024 at 16:23):alexcrichton has enabled auto merge for PR #9765.
Wasmtime GitHub notifications bot (Dec 10 2024 at 16:30):alexcrichton updated PR #9765.
Wasmtime GitHub notifications bot (Dec 10 2024 at 17:05):alexcrichton merged PR #9765.
Last updated: Dec 23 2024 at 12:05 UTC