cranelift-wasm: Stop using table_addr instructions #8063
Conversation
|
I'm quite excited for this 👍, thanks for taking this on! |
github-actions
bot
added
cranelift
jameysharp
force-pushed
the
stop-using-tables
branch
from
514bdb7 to
5682a53
Compare
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
jameysharp
force-pushed
the
stop-using-tables
branch
from
5682a53 to
eaf08ee
Compare
jameysharp
changed the title
| } else if element_size.is_power_of_two() { | ||
| pos.ins() | ||
| .ishl_imm(index, i64::from(element_size.trailing_zeros())) | ||
| } else { | ||
| pos.ins().imul_imm(index, element_size as i64) | ||
| }; |
There was a problem hiding this comment.
Not 100% convinced it is worth adding the power-of-2 optimization here, since our mid-end rules should definitely already handle this. I think this file should focus on just (a) the correctness of the bounds checks and (b) optimizations to the bounds checks that rely on knowledge of the Wasm table and its limits that the mid-end can't possibly know.
There was a problem hiding this comment.
And apologies if tihs already existed. I think we can clean up a tiny bit of complexity here, if that was the case.
There was a problem hiding this comment.
Yeah, I considered removing that in favor of relying on the corresponding egraph optimizations, but decided to keep it since it was in the original. I'd prefer to land it in this form to avoid bigger changes to the filetests, where I specifically have not turned on the egraph pass so it's more clear what's being generated, but I wouldn't be upset about removing this later.
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.