-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
interpreter,compiler(amd64): complete SIMD instructions #624
Conversation
ok passed |
ok |
ok passed all spec test with the interpreter, meaning that it is fully compatible with WebAssembly 2.0 draft! |
ok passed all tests for both engines... backfilling comments and unit tests... |
8cd5b5f
to
79bd254
Compare
02438ad
to
3cbfac9
Compare
ok finished the tests for spectest.go... marked this for ready! |
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unleashing a few
// https://www.felixcloutier.com/x86/pmulhrsw | ||
PMULHRSW: {mandatoryPrefix: 0x66, opcode: []byte{0x0f, 0x38, 0x0b}, requireSrcFloat: true, requireDstFloat: true}, | ||
// https://www.felixcloutier.com/x86/pmovsx | ||
PMOVSXBW: {mandatoryPrefix: 0x66, opcode: []byte{0x0f, 0x38, 0x20}, requireSrcFloat: true, requireDstFloat: true}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add something like this to registerToRegisterOpcode
to help next person figure out when an instruction is missing ;) I don't know the correct words, so gave it a shot.
ex.
// The index of felixcloutier.com does not include all instruction variations.
// For example, removing the suffix `BW` from `PMOVSXBW` allows you to find it in https://www.felixcloutier.com/x86/index.html.
//
// Here are the known instruction conventions used in felixcloutier.com
// * BW - 16-bit integer
// * BD - 32-bit integer
// * BQ - 64-bit integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice we are getting to tens of thousands of tests due to matrix t.Run
While this is a good default, I think that in the case of instruction boundary testing, especially later with fuzzing, we can do it differently to keep things like IDEs from crashing, and also keep the runtime lower.
Basically substitute dimensional explosions with a format string on input instead of t.Run
ex. require.False(t, x, "input %d", y)
Then in RATIONALE.md, say...
Test matrices
Typically, we advice boundary tests to use go test tables, where t.Run
is run repeatedly with different inputs, using pinned variables. An exception to this are compiler instruction tests, where there are too many inputs, resulting in tens of thousands of tests.
Instead, we run a normal loop substituting dimensional explosions with a format string on input instead of t.Run
ex. require.False(t, x, "input %d", y)
This keeps the test cardinality low which reduces runtime. On the rare failure, the input is visible in the failure output. While re-running this input is less ideal than a separate test, it is possible manually by temporarily removing the loop.
name: "pmullw xmm13, xmm1", | ||
n: &NodeImpl{ | ||
Instruction: PMULLW, | ||
SrcReg: RegX1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this file, maybe order dest first so that it doesn't break the brain that the name is the other way ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great, especially as I'm sure arm64 is gonna be easier!
main thing to do now or later is pay down some techdebt accruing in interpreter.go. I feel this part of the codebase may see some contributions by external folks, so it could be a bit more inviting.
@@ -330,7 +330,7 @@ const ( | |||
nativeCallStatusIntegerDivisionByZero | |||
) | |||
|
|||
// causePanic causes a panic with the corresponding error to the status code. | |||
// causePanic causes a panic with the corresponding error to the nativeCallStatusCode. | |||
func (s nativeCallStatusCode) causePanic() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed in go Panic()
functions.. this avoids clash on panic
food for thought.
Co-authored-by: Crypt Keeper <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
This completes the implementation of arm64 backend for SIMD instructions. Notably, now the arm64 compiler passes 100% of WebAssemby 2.0 draft specification tests. Combined with the completion of the interpreter and amd64 backend (#624), this finally resolves #484. Therefore, this also documents that wazero is 100% compatible with WebAssembly 1.0 and 2.0. Signed-off-by: Takeshi Yoneda <[email protected]>
This completes the implementation of SIMD proposal for both
the interpreter and compiler(amd64).
This also fixes #210 by adding the complete documentation
over all the wazeroir operations.
arm64 compiler will follow in subsequent PRs.
part of #484