-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
winch: Add support for WebAssembly loads/stores #7894
winch: Add support for WebAssembly loads/stores #7894
Conversation
Closes bytecodealliance#6529 This patch adds support for all the instructions involving WebAssembly loads and stores for 32-bit memories. Given that the `memory64` proposal is not enabled by default, this patch doesn't include an implementation/tests for it; in theory minimal tweaks to the currrent implementation will be needed in order to support 64-bit memories. Implemenation-wise, this change, follows a similar pattern as Cranelift in order to calculate addresses for dynamic/static heaps, the main difference being that in some cases, doing less work at compile time is preferred; the current implemenation only checks for the general case of out-of-bounds access for dynamic heaps for example. Another important detail regarding the implementation, is the introduction of `MacroAssembler::wasm_load` and `MacroAssembler::wasm_store`, which internally use a common implemenation for loads and stores, with the only difference that the `wasm_*` variants set the right flags in order to signal that these operations are not trusted and might trap. Finally, given that this change introduces support for the last set of instructions missing for a Wasm MVP, it removes most of Winch's copy of the spectest suite, and switches over to using the official test suite where possible (for tests that don't use SIMD or Reference Types). Follow-up items: * Before doing any deep benchmarking I'm planning on landing a couple of improvements regarding compile times that I've identified in parallel to this change. * The `imports.wast` tests are disabled because I've identified a bug with `call_indirect`, which is not related to this change and exists in main. * Find a way to run the `tests/all/memory.rs` (or perhaps most of integration tests) with Winch. -- prtest:full
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "fuzzing", "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So happy to see this PR, congratulations on closing out #6528!
The structure of this change makes sense to me, so I went ahead and made a bunch of smaller suggestions.
@elliottt Thanks for the detailed review and all the great suggestions, I've resolved most of them, excluding two of them which I've left open for further discussion. Happy to continue the discussion and/or create follow-up PRs! |
Closes #6528
This patch adds support for all the instructions involving WebAssembly loads and stores for 32-bit memories. Given that the
memory64
proposal is not enabled by default, this patch doesn't include an implementation/tests for it; in theory minimal tweaks to the currrent implementation will be needed in order to support 64-bit memories.Implemenation-wise, this change, follows a similar pattern as Cranelift in order to calculate addresses for dynamic/static heaps, the main difference being that in some cases, doing less work at compile time is preferred; the current implemenation only checks for the general case of out-of-bounds access for dynamic heaps for example.
Another important detail regarding the implementation, is the introduction of
MacroAssembler::wasm_load
andMacroAssembler::wasm_store
, which internally use a common implemenation for loads and stores, with the only difference that thewasm_*
variants set the right flags in order to signal that these operations are not trusted and might trap.Finally, given that this change introduces support for the last set of instructions missing for a Wasm MVP, it removes most of Winch's copy of the spectest suite, and switches over to using the official test suite where possible (for tests that don't use SIMD or Reference Types).
Follow-up items:
imports.wast
tests are disabled because I've identified a bug withcall_indirect
, which is not related to this change and exists in main.tests/all/memory.rs
(or perhaps most of integration tests) with Winch.--
prtest:full