Incorrect codegen for i16x8.extmul_high_i8x16_s on x64

[alexcrichton]

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

[abrown]

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.

[alexcrichton]

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)

[jlb6740]

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.

[jlb6740]

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.

[alexcrichton]

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

[jlb6740]

@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.

[jlb6740]

@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.

[akirilov-arm]

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

[alexcrichton]

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.

[jlb6740]

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?

[alexcrichton]

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 :(

[jlb6740]

Also submitted a pr here: WebAssembly/testsuite#41 for an update of the spec tests.