Skip to content
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

Update relaxed simd instructions #766

Merged
merged 5 commits into from Sep 16, 2022

Conversation

yurydelendik
Copy link
Contributor

Changes wasm-tools to implement current https://github.com/WebAssembly/relaxed-simd/blob/main/proposals/relaxed-simd/Overview.md

  • Update instructions names and codes
  • Add new instructions i16x8.relaxed_q15mulr_s, i16x8.dot_i8x16_i7x16_s, i32x4.dot_i8x16_i7x16_add_s, and f32x4.relaxed_dot_bf16x8_add_f32x4

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

Out of curiosity, I believe your main motivation for this is updating the text parser for the usage in Firefox, is that right? If so, do you find it onerous/difficult/annoying to update all the other crates in the workspace? I find we have a lot here so updating little things can be a surprising amount of effort and I'm always hoping we can find more central locations to define all of this. Recently I'm wondering if we should try to deduplicate all the instruction encodings/names into one place to avoid having to name them in so many places. Anyway just curious, always happy to merge these PRs!

@alexcrichton alexcrichton merged commit 5db14d0 into bytecodealliance:main Sep 16, 2022
@alexcrichton
Copy link
Member

Oh also, would you like a publish?

@yurydelendik
Copy link
Contributor Author

If so, do you find it onerous/difficult/annoying to update all the other crates in the workspace?
Recently I'm wondering if we should try to deduplicate all the instruction encodings/names into one place to avoid having to name them in so many places.

It is not a big deal to update in multiple place, as long as testing will check typos and discrepancies in implementations.

Oh also, would you like a publish?

Yes, at your earliest convenience. I anticipate a reasonable delay up to 2 weeks between version release.

@alexcrichton
Copy link
Member

@yurydelendik I'm not following the the relaxed-simd spec much so you may know the answer to this. With WebAssembly/testsuite#62 I was testing out updating the wasm-tools submodule to the testsuite to include relaxed-simd to ensure that all tests pass. Currently the only error is:

failed test: tests/testsuite/proposals/relaxed-simd/relaxed_fma_fms.wast

Caused by:
    unknown operator or unexpected token
         --> tests/testsuite/proposals/relaxed-simd/relaxed_fma_fms.wast:5:78
          |
        5 |     (func (export "f32x4.relaxed_fms") (param v128 v128 v128) (result v128) (f32x4.relaxed_fms (local.get 0) (local.get 1) (local.get 2)))
          |                                                                              ^

It looks like fms operations were reimplemented as fnma but there aren't any spec tests for fnma, so I'm wondering if you know what the current state of this is and how to reconcile this.

@alexcrichton
Copy link
Member

Also I'm happy to publish whenever, I'm just waiting for a day with no fuzz-bugs to publish. (should be today, checking on oss-fuzz now)

@yurydelendik
Copy link
Contributor Author

It looks like fms operations were reimplemented as fnma but there aren't any spec tests for fnma

The difference between f32x4.relaxed_fms and f32x4.relaxed_fnma is operands order. The existing test will be changed to use third operand as an accumulator (in fms and fma it was the first one). The old f32x4.relaxed_fma test will be failing as well. This change was made to keep signature similar to dot instructions.

Also I'm happy to publish whenever, I'm just waiting for a day with no fuzz-bugs to publish.

No problem. It is sort of a breaking change, and probably has not good tests. Disabling tests for fma/fms can be an option.

@alexcrichton
Copy link
Member

Ok sounds good, I'll handle that accordingly and publish these changes today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants