Implement Relaxed SIMD proposal #1994
Conversation
marcusb
force-pushed
the
relaxed-simd
branch
2 times, most recently
from
f9ceacd to
821b0a5
Compare
|
Thank you for doing this! I'm probably not the most qualified person to review this, but, as a starter, I don't think we want to vendor the relaxed-simd test scripts into wabt unless we really have to. My suggestion would be to submit a PR to add relaxed-simd to the testsuite repo list (https://github.com/WebAssembly/testsuite/blob/main/update-testsuite.sh#L9) and then add them there, and then coming back here with a PR that updates the testsuite submodule, and then we go from there. |
|
Ok, added to testsuite in WebAssembly/testsuite#62 |
marcusb
force-pushed
the
relaxed-simd
branch
from
821b0a5 to
bafd8c4
Compare
|
Those test changes don't look like a problem, but we should probably update the testsuite in a separate PR... For the non-wasm2c tests, it looks like they just need to be rebased. For the wasm2c tests, either this will need to happen after #1877 (in which case updating the test suite is just a rebase too) or more tests will need to be frozen and moved to old-spec. |
marcusb
force-pushed
the
relaxed-simd
branch
from
bafd8c4 to
31669d1
Compare
|
The Relaxed SIMD proposal is still in flux so there will be some changes down the line:
|
src/interp/interp.cc
Outdated
| case O::F32X4Min: return DoSimdBinop(FloatMin<f32>); | ||
| case O::F32X4Max: return DoSimdBinop(FloatMax<f32>); | ||
| case O::F32X4Div: | ||
| return DoSimdBinop(FloatDiv<f32>); |
There was a problem hiding this comment.
Why not keep this on a single line like the others? It doesn't looks like we are near the line limit.
There was a problem hiding this comment.
This was done by clang-format and enforced by the linting, if I'm not mistaken.
There was a problem hiding this comment.
This seems unfortunate.. I guess we should exclude this whole block from clang-formatting.
src/opcode-code-table.h
Outdated
| /* | ||
| * Number of bits required to store an opcode | ||
| */ | ||
| #define MAX_OPCODE_BITS 9 |
There was a problem hiding this comment.
Is this an intentional part of the relaxed simd proposal, that the opcode space is now larger and a byte?
There was a problem hiding this comment.
The vector instructions were always using variable-length integer opcodes, but these are the only instructions in this implementation with opcodes above 0xff. This necessitates a change in the structure of the internal lookup table (and also unfortunately doubles its size to 128 kB).
There was a problem hiding this comment.
Just to be clear, adding relaxed SIMD introduces opcode whose encoding is > 0xff for the first time?
There was a problem hiding this comment.
Yes, to be precise we're adding prefixed opcodes with suffixes over 0xff.
marcusb
force-pushed
the
relaxed-simd
branch
from
31669d1 to
62ead3f
Compare
|
@marcusb Do you have cycles to respond to the review (which might just be about formatting at this point) and rebase? This seems close and would be great to have it in... |
|
@keithw I'll bring the PR up to date with the latest changes in the spec. Here is the testsuite bump PR for a start. |
marcusb
force-pushed
the
relaxed-simd
branch
from
62ead3f to
812176e
Compare
|
@keithw Seems there are a few unrelated test failures after pulling in the latest testsuite, eg some elem-type mismatches. |
I've been working on a PR to update the testsuite .. along with the corresponding fixes. Uploading now. |
marcusb
force-pushed
the
relaxed-simd
branch
from
812176e to
1f14c7d
Compare
|
Rebased and brought up to date with spec. |
|
Ok to merge this now? |
|
Ok to merge as far as I'm concerned. |
|
Ok, merging. Hope there's no objection, but I'd like to copy the note from the commit message (about the missing f32x4.relaxed_dot_bf16x8_add_f32x4 instruction) to a footnote in the README so we don't forget about it and nobody gets surprised if they try to use that instruction and get an error. Of course we should remove the note and add the remaining test as soon as we implement the instr (and I assume the proposal may continue to evolve anyway). |
|
Sigh, I thought I was going to be able to merge this but I guess we have to wait until after #2082. |
This adds support for the new opcodes from the Relaxed SIMD proposal (https://github.com/WebAssembly/relaxed-simd) behind the "--enable-relaxed-simd" flag. The exception is the f32x4.relaxed_dot_bf16x8_add_f32x4 instruction which is not yet implemented.
This adds support for the new opcodes from the Relaxed SIMD proposal (https://github.com/WebAssembly/relaxed-simd) behind the "--enable-relaxed-simd" flag. The exception is the f32x4.relaxed_dot_bf16x8_add_f32x4 instruction which is not yet implemented.
This adds support for the new opcodes from the Relaxed SIMD proposal (https://github.com/WebAssembly/relaxed-simd) behind the "--enable-relaxed-simd" flag.