Stream: git-wasmtime

Topic: wasmtime / PR #8063 Stop using tables


view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2024 at 23:14):

jameysharp opened PR #8063 from jameysharp:stop-using-tables to bytecodealliance:main:

This PR fixes #5532, removing a bunch of code from Cranelift and adding only a little bit of code to the wasmtime-cranelift crate.

I want to implement something like the wasm heap filetests for tables and land that before this, so I'm opening this as a draft. This PR should have no effect on generated code and it'd be nice to verify that.

Merging all the table-related code into one place should eventually let us experiment more with bounds-check elimination tricks, like we've been doing with heaps.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2024 at 23:41):

alexcrichton commented on PR #8063:

I'm quite excited for this :+1:, thanks for taking this on!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2024 at 16:04):

fitzgen submitted PR review:

Also excited for this!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2024 at 16:04):

fitzgen created PR review comment:

When removing memories from CLIF, we pushed them into cranelift-wasm and not wasmtime-cranelift.

At a high level, this decision makes sense to me because memories are a general Wasm concept and not a Wasmtime-specific concept.

In this PR, you're pushing tables all the way out to wasmtime-cranelift, rather than just to cranelift-wasm. I'm wondering what the motivation for this was? Because tables are a general Wasm concept, not a Wasmtime-specific concept, so I would expect them to be in cranelift-wasm.

More concretely, we would ideally share all the various bounds-checking logic that is in cranelift/wasm/src/code_translator/bounds_checks.rs between Wasm tables and memories, and that seems like it will be easier if tables are moved to the same location as memories.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2024 at 16:04):

fitzgen submitted PR review:

Also excited for this!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2024 at 17:52):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2024 at 17:52):

jameysharp created PR review comment:

The motivation was that this was what I could do in one day of hacking to learn about the problem space. :grin: I started by copying the implementation of the table_addr legalization pass to the only module which used table_addr, and that happened to be in wasmtime-cranelift.

In the process, I figured out that if I want to use the filetests infrastructure to test table instructions like you did for memory instructions, then I'm pretty much forced to push this code into cranelift-wasm instead.

That's going to require some more thought about exactly what API is needed, but I totally agree that it's worth doing.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2024 at 18:26):

jameysharp updated PR #8063.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2024 at 19:20):

jameysharp updated PR #8063.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 13 2024 at 19:23):

jameysharp edited PR #8063:

This CLIF instruction is specific to WebAssembly, and doesn't expose any optimization opportunities or have any other reason to be treated specially by Cranelift. So this commit makes Wasmtime emit a series of simpler Cranelift instructions instead.

I copied the implementation from Cranelift's legalization pass, which already was rewriting these instructions to simpler ones, and then simplified the result to not generate the intermediate form at all.

Merging all the table-related code into one place should eventually let us experiment more with bounds-check elimination tricks, like we've been doing with heaps.

The filetest changes demonstrate that the new cranelift-wasm implementation generates the exact same sequence of instructions that the legalization pass did, except with different value numbers and fewer aliases.

Several features of Cranelift's table support were unused, so while copying parts of Cranelift into this crate I removed those features. Specifically:

This is a step toward #5532.


Last updated: Dec 23 2024 at 12:05 UTC