Stream: git-wasmtime

Topic: wasmtime / issue #3089 Incorrect codegen for i16x8.extmul...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 15:43):

alexcrichton labeled issue #3089:

I was writing some tests for Rust's simd support recently and I think that the codegen for one of the recently added extmul instructions may be incorrect:

(module
  (func $inputs (result v128 v128)
      v128.const i8x16 -1 -2 3 100 124 -38 33 87 92 108 22 8 -43 -128 22 0
      v128.const i8x16 -5 -2 6 10 45 -4 4 -2 0 88 92 -102 -98 83 73 54)

  (func (export "low") (result v128)
      call $inputs
      i16x8.extmul_low_i8x16_s)
  (func (export "high") (result v128)
      call $inputs
      i16x8.extmul_high_i8x16_s)
)

$ ../wasmtime/target/release/wasmtime foo.wat --enable-simd --invoke low
warning: using `--invoke` with a function that returns values is experimental and may break in the future
339378917725854714996597693406159044613
$ ../wasmtime/target/release/wasmtime foo.wat --enable-simd --invoke high
warning: using `--invoke` with a function that returns values is experimental and may break in the future
339378917725854714996597693406159044613

It looks like these two instructions are producing the same result, but I don't think they should be the same?

cc @jlb6740 @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 15:43):

alexcrichton opened issue #3089:

I was writing some tests for Rust's simd support recently and I think that the codegen for one of the recently added extmul instructions may be incorrect:

(module
  (func $inputs (result v128 v128)
      v128.const i8x16 -1 -2 3 100 124 -38 33 87 92 108 22 8 -43 -128 22 0
      v128.const i8x16 -5 -2 6 10 45 -4 4 -2 0 88 92 -102 -98 83 73 54)

  (func (export "low") (result v128)
      call $inputs
      i16x8.extmul_low_i8x16_s)
  (func (export "high") (result v128)
      call $inputs
      i16x8.extmul_high_i8x16_s)
)

$ ../wasmtime/target/release/wasmtime foo.wat --enable-simd --invoke low
warning: using `--invoke` with a function that returns values is experimental and may break in the future
339378917725854714996597693406159044613
$ ../wasmtime/target/release/wasmtime foo.wat --enable-simd --invoke high
warning: using `--invoke` with a function that returns values is experimental and may break in the future
339378917725854714996597693406159044613

It looks like these two instructions are producing the same result, but I don't think they should be the same?

cc @jlb6740 @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 15:43):

alexcrichton labeled issue #3089:

I was writing some tests for Rust's simd support recently and I think that the codegen for one of the recently added extmul instructions may be incorrect:

(module
  (func $inputs (result v128 v128)
      v128.const i8x16 -1 -2 3 100 124 -38 33 87 92 108 22 8 -43 -128 22 0
      v128.const i8x16 -5 -2 6 10 45 -4 4 -2 0 88 92 -102 -98 83 73 54)

  (func (export "low") (result v128)
      call $inputs
      i16x8.extmul_low_i8x16_s)
  (func (export "high") (result v128)
      call $inputs
      i16x8.extmul_high_i8x16_s)
)

$ ../wasmtime/target/release/wasmtime foo.wat --enable-simd --invoke low
warning: using `--invoke` with a function that returns values is experimental and may break in the future
339378917725854714996597693406159044613
$ ../wasmtime/target/release/wasmtime foo.wat --enable-simd --invoke high
warning: using `--invoke` with a function that returns values is experimental and may break in the future
339378917725854714996597693406159044613

It looks like these two instructions are producing the same result, but I don't think they should be the same?

cc @jlb6740 @abrown

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 16:49):

abrown commented on issue #3089:

Hm, just checked that the SIMD spec tests are enabled and it looks like they are. If @jlb6740 can confirm this is indeed a bug in #3084 then we need to fix the spec tests as well.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 16:49):

abrown edited a comment on issue #3089:

Hm, just checked that the SIMD spec tests for these instructions are enabled and it looks like they are. If @jlb6740 can confirm this is indeed a bug in #3084 then we need to fix the spec tests as well.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 16:55):

alexcrichton commented on issue #3089:

When I looked at the spec tests it looks like they never exercised the upper/lower halves actually being different. It appeared that if an engine implemented the these two instructions in the same way it'd pass the spec tests (not that wasmtime does this, and agreed that the spec tests should improve to catch this)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 16:55):

jlb6740 commented on issue #3089:

Yes .. I think a lot of spec tests are inadequate. For this one I modified parts of my implementation to do an incorrect lowering and it would still pass. Didn't want to not push it do to limited spec tests since it's not the only one.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 16:56):

jlb6740 edited a comment on issue #3089:

Yes .. I think a lot of spec tests are inadequate. For this one I modified parts of my implementation to do an incorrect lowering and it would still pass. Didn't want to not push it though due to limited spec tests since it's not the only one.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 16:57):

jlb6740 commented on issue #3089:

I'll try to fix .. If I can't in the next 30 min I'll revert since I won't have much other time before this evening.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 17:08):

alexcrichton commented on issue #3089:

Oh I don't think there's any need to revert, I just wanted to report this to have it on file!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 17:17):

jlb6740 commented on issue #3089:

@alexcrichton Yes ... thanks Alex. I guess it isn't that serious but it may be a second before I get the fix. @abrown is right, I noticed the spec tests were inadequate but that didn't mean the lowering wasn't sound. Need simd fuzz testing. Also, I had trouble printing out the sequence .. passing --set has_ssse3=true --set has_sse41=true was an issue for the command:

"RUST_LOG=DEBUG cargo run -- wasm --target x86_64 --set has_ssse3=true --set has_sse41=true -dDpv vcode_x64_examples.wat.bak".

Related .. do you know how to pass more than one architecture to cranelift.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 17:29):

jlb6740 commented on issue #3089:

@alexcrichton Nevermind. Just got the command with @abrown. This is good:

cargo run -p cranelift-tools -- wasm --set="enable_simd=true" --target x86_64 -dDv scratch.wat

has_* is needed no more.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 18:38):

akirilov-arm commented on issue #3089:

@sparker-arm, you should check this test case before merging #3070.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2021 at 21:13):

alexcrichton commented on issue #3089:

In testing I think that this is also an issue for the unsigned variant -- i16x8.extmul_high_i8x16_u, but not for any other widths of extmul instructions.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2021 at 17:01):

jlb6740 commented on issue #3089:

Hi all @alexcrichton @sparker-arm @abrown. I saw this error during my refactoring of the the extmul patch which may have introduced an incorrect lowering for i16x8.extmul_high_i8x16_u:

Downloaded fst v0.4.6
error: failed to download from https://crates.io/api/v1/crates/crossbeam-utils/0.8.5/download

Caused by:
[55] Failed sending data to the peer (Connection died, tried 5 times before giving up)
Error: Process completed with exit code 101.

And based on the error assumed it was unrelated to the patch. Is it possible that the failed sending is related to this patch that has an incorrect lowering? If so then perhaps I can just submit a patch to ignore this test path? Also,

cargo run -p cranelift-tools -- wasm --set="enable_simd=true" --target x86_64 -dDv scratch.wat

Did not work for allowing multiple ISA requirements (such as both sse41 and ssse3). Is there a way to specify this on the command line?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2021 at 17:03):

jlb6740 edited a comment on issue #3089:

Hi all @alexcrichton @sparker-arm @abrown. I saw this error during my refactoring of the the extmul patch which may have introduced an incorrect lowering for i16x8.extmul_high_i8x16_u:

Downloaded fst v0.4.6
error: failed to download from https://crates.io/api/v1/crates/crossbeam-utils/0.8.5/download

Caused by:
[55] Failed sending data to the peer (Connection died, tried 5 times before giving up)
Error: Process completed with exit code 101.

And based on the error assumed it was unrelated to the patch. Is it possible that the failed sending is related to this patch that has an incorrect lowering? If so then perhaps I can just submit a patch to ignore this test path or at least market it as should panic on x64? Also,

cargo run -p cranelift-tools -- wasm --set="enable_simd=true" --target x86_64 -dDv scratch.wat

Did not work for allowing multiple ISA requirements (such as both sse41 and ssse3). Is there a way to specify this on the command line?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2021 at 17:19):

alexcrichton commented on issue #3089:

Nah the send error is a Cargo bug that should be fixed on nightly soon, as for ISA requirements I'm not sure myself, I don't know much about the cranelift tools :(

view this post on Zulip Wasmtime GitHub notifications bot (Jul 18 2021 at 23:51):

jlb6740 commented on issue #3089:

Also submitted a pr here: https://github.com/WebAssembly/testsuite/pull/41 for an update of the spec tests.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 12 2021 at 00:43):

alexcrichton commented on issue #3089:

I think this has since been fixed.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 12 2021 at 00:43):

alexcrichton closed issue #3089:

I was writing some tests for Rust's simd support recently and I think that the codegen for one of the recently added extmul instructions may be incorrect:

(module
  (func $inputs (result v128 v128)
      v128.const i8x16 -1 -2 3 100 124 -38 33 87 92 108 22 8 -43 -128 22 0
      v128.const i8x16 -5 -2 6 10 45 -4 4 -2 0 88 92 -102 -98 83 73 54)

  (func (export "low") (result v128)
      call $inputs
      i16x8.extmul_low_i8x16_s)
  (func (export "high") (result v128)
      call $inputs
      i16x8.extmul_high_i8x16_s)
)

$ ../wasmtime/target/release/wasmtime foo.wat --enable-simd --invoke low
warning: using `--invoke` with a function that returns values is experimental and may break in the future
339378917725854714996597693406159044613
$ ../wasmtime/target/release/wasmtime foo.wat --enable-simd --invoke high
warning: using `--invoke` with a function that returns values is experimental and may break in the future
339378917725854714996597693406159044613

It looks like these two instructions are producing the same result, but I don't think they should be the same?

cc @jlb6740 @abrown

Last updated: Nov 22 2024 at 16:03 UTC