saulecabrera opened PR #7894 from saulecabrera:winch-loads-stores
to bytecodealliance:main
:
Closes https://github.com/bytecodealliance/wasmtime/issues/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 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:
- 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 withcall_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<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
saulecabrera requested elliottt for a review on PR #7894.
saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7894.
saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #7894.
saulecabrera requested pchickey for a review on PR #7894.
saulecabrera requested wasmtime-core-reviewers for a review on PR #7894.
saulecabrera requested wasmtime-default-reviewers for a review on PR #7894.
github-actions[bot] commented on PR #7894:
Subscribe to Label Action
cc @fitzgen, @saulecabrera
<details>
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:
- fitzgen: fuzzing
- saulecabrera: winch
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
saulecabrera edited PR #7894:
Closes https://github.com/bytecodealliance/wasmtime/issues/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
and
MacroAssembler::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:
- 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 withcall_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<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
elliottt submitted PR review:
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 submitted PR review:
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 created PR review comment:
index
is passed out unmodified as the first result here, would it be reasonable to change this return type to justImmOffset
?) -> ImmOffset {
elliottt created PR review comment:
Would it make sense to invert this case, and name the unsupported ops at this point? That would also give us a nice list for what ops still need to be implemented.
elliottt created PR review comment:
Is this argument necessary? There's already a
masm
argument, which could be used to callscratch_reg
again.
elliottt created PR review comment:
/// Loads the bounds of the dynamic heap.
elliottt created PR review comment:
This pattern shows up in a few places, would it be easy to special case the
0
offset and avoid theadd
?
elliottt created PR review comment:
What about casting where
ptr_size
is defined instead? I think would leave all other uses ofptr_size
unchanged.
elliottt created PR review comment:
Was this change unintentional?
elliottt created PR review comment:
/// Load the requested heap address into the specified destination register.
elliottt created PR review comment:
I think you can remove the
<M>
type argument by changing how theMasm
impl is referenced (unless the intent is to reuse this implementation with a differentMasm
impl in the future). I like this a little bit more, as seeingself.store_impl<Self>
at a call site was a little surprising :)fn store_impl(&mut self, src: RegImm, dst: Address, size: OperandSize, flags: MemFlags) { let scratch = <Self as Masm>::ABI::scratch_reg(); let float_scratch = <Self as Masm>::ABI::float_scratch_reg();
elliottt created PR review comment:
What do you think about calling this function
load_heap_addr_unchecked
? Similarly, we could renamecheck_addr
toload_heap_addr_checked
so that it's clear what the difference between the two is?
elliottt created PR review comment:
The
trusted_flags
field doesn't look like it's modified once created, are there plans in the future to mutate this throughout the lifetime of theMacroAssembler
, or would this be better extracted out as a constant?
saulecabrera updated PR #7894.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
I've done a small refactoring here, and aside from inverting the list, I've disabled the Wasm proposals that are not supported yet.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Yeah, makes sense to me!
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Yeah good call, I do like the proposed naming better.
saulecabrera created PR review comment:
Good catch, yeah definitely unintentional.
saulecabrera submitted PR review.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Yeah, this could definitely be improved I guess, the current implementation is already an improvement to what was there before in the sense that we were creating a
MemFlags::trusted
per load / store, now it's created once per MacroAssembler (and by definition once per function). Given that the flags won't be modified, I think that sharing both (trusted and untrusted) through all the functions might make sense. That said, it was not evident to me how to go about the constant route, were you thinking about introducing something likeOnceCell
? Having a pure constant won't work given thatMemFlags::trusted
is notconst
. Another approach could be to create aMacroAssemblerContext
that is created verly early in the process and passed in to eachMacroAssembler
implementation and reused across all the functions.
saulecabrera submitted PR review.
saulecabrera created PR review comment:
Yeah, a bit unfortunate, there are a couple of patterns like this throughout Winch. I was hoping that we would be able to get rid of them once we start exploring the optimizations that we could apply ; right now this would require introducing at least a new abstraction at the
CodeGenContext
to enable peeking at constants at certain points of the code generation. This is not too difficult I think, but before getting into any optimizations for the generated code, I was hoping to make all the possible improvements to the compilation times to have a good sense of what our baseline is.
saulecabrera edited PR review comment.
saulecabrera edited PR review comment.
saulecabrera commented on PR #7894:
@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!
saulecabrera merged PR #7894.
elliottt submitted PR review.
elliottt created PR review comment:
I made a change that should allow this in #7903 -- it turned out to be easier than I expected to make the MemFlags have a
const
-compatible interface :)
Last updated: Dec 23 2024 at 12:05 UTC