Wasmtime GitHub notifications bot (Mar 07 2024 at 23:14): 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.
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!
Wasmtime GitHub notifications bot (Mar 08 2024 at 16:04):fitzgen submitted PR review:
Also excited for this!
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-wasmand notwasmtime-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 tocranelift-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 incranelift-wasm.More concretely, we would ideally share all the various bounds-checking logic that is in
cranelift/wasm/src/code_translator/bounds_checks.rsbetween Wasm tables and memories, and that seems like it will be easier if tables are moved to the same location as memories.
Wasmtime GitHub notifications bot (Mar 08 2024 at 16:04):fitzgen submitted PR review:
Also excited for this!
Wasmtime GitHub notifications bot (Mar 08 2024 at 17:52):jameysharp submitted PR review.
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_addrlegalization pass to the only module which usedtable_addr, and that happened to be inwasmtime-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-wasminstead.That's going to require some more thought about exactly what API is needed, but I totally agree that it's worth doing.
Wasmtime GitHub notifications bot (Mar 13 2024 at 18:26):jameysharp updated PR #8063.
Wasmtime GitHub notifications bot (Mar 13 2024 at 19:20):jameysharp updated PR #8063.
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:
- TableData's min_size and index_type fields
- table_addr's immediate byte-offset operand
This is a step toward #5532.
Last updated: Oct 23 2024 at 20:03 UTC