wasmtime / PR #7894 winch: Add support for WebAssembly lo... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7894 winch: Add support for WebAssembly lo...

Wasmtime GitHub notifications bot (Feb 08 2024 at 12:45):

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 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

Wasmtime GitHub notifications bot (Feb 08 2024 at 12:45):

saulecabrera requested elliottt for a review on PR #7894.

Wasmtime GitHub notifications bot (Feb 08 2024 at 12:45):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7894.

Wasmtime GitHub notifications bot (Feb 08 2024 at 12:45):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #7894.

Wasmtime GitHub notifications bot (Feb 08 2024 at 12:45):

saulecabrera requested pchickey for a review on PR #7894.

Wasmtime GitHub notifications bot (Feb 08 2024 at 12:45):

saulecabrera requested wasmtime-core-reviewers for a review on PR #7894.

Wasmtime GitHub notifications bot (Feb 08 2024 at 12:45):

saulecabrera requested wasmtime-default-reviewers for a review on PR #7894.

Wasmtime GitHub notifications bot (Feb 08 2024 at 13:45):

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>

Wasmtime GitHub notifications bot (Feb 08 2024 at 13:49):

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 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

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

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.

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

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.

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

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 just ImmOffset?
) -> ImmOffset {

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

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.

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

elliottt created PR review comment:

Is this argument necessary? There's already a masm argument, which could be used to call scratch_reg again.

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

elliottt created PR review comment:

/// Loads the bounds of the dynamic heap.

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

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 the add?

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

elliottt created PR review comment:

What about casting where ptr_size is defined instead? I think would leave all other uses of ptr_size unchanged.

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

elliottt created PR review comment:

Was this change unintentional?

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

elliottt created PR review comment:

/// Load the requested heap address into the specified destination register.

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

elliottt created PR review comment:

I think you can remove the <M> type argument by changing how the Masm impl is referenced (unless the intent is to reuse this implementation with a different Masm impl in the future). I like this a little bit more, as seeing self.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();

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

elliottt created PR review comment:

What do you think about calling this function load_heap_addr_unchecked? Similarly, we could rename check_addr to load_heap_addr_checked so that it's clear what the difference between the two is?

Wasmtime GitHub notifications bot (Feb 08 2024 at 23:58):

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 the MacroAssembler, or would this be better extracted out as a constant?

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:55):

saulecabrera updated PR #7894.

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:57):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:57):

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.

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:57):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:57):

saulecabrera created PR review comment:

Yeah, makes sense to me!

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:58):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:58):

saulecabrera created PR review comment:

Yeah good call, I do like the proposed naming better.

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:59):

saulecabrera created PR review comment:

Good catch, yeah definitely unintentional.

Wasmtime GitHub notifications bot (Feb 09 2024 at 14:59):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 15:05):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 15:05):

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 like OnceCell? Having a pure constant won't work given that MemFlags::trusted is not const. Another approach could be to create a MacroAssemblerContext that is created verly early in the process and passed in to each MacroAssembler implementation and reused across all the functions.

Wasmtime GitHub notifications bot (Feb 09 2024 at 15:09):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 15:09):

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.

Wasmtime GitHub notifications bot (Feb 09 2024 at 15:12):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Feb 09 2024 at 15:12):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Feb 09 2024 at 15:13):

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!

Wasmtime GitHub notifications bot (Feb 09 2024 at 16:09):

saulecabrera merged PR #7894.

Wasmtime GitHub notifications bot (Feb 09 2024 at 16:50):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2024 at 16:50):

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: Oct 23 2024 at 20:03 UTC