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

wasm simd: folded xor and unaligned load incorrectly "traps" #2943

Closed
alexcrichton opened this issue May 27, 2021 · 1 comment · Fixed by #3107
Closed

wasm simd: folded xor and unaligned load incorrectly "traps" #2943

alexcrichton opened this issue May 27, 2021 · 1 comment · Fixed by #3107
Assignees
Labels
bug Incorrect behavior in the current implementation that needs fixing wasm-proposal:simd Issues related to the WebAssembly SIMD proposal

Comments

@alexcrichton
Copy link
Member

Given this wasm:

(module
  (memory 1)
  (func (export "_start")
        call 1
        drop)

  (func (result v128)
    v128.const i32x4 0 0 0 0
    i32.const 1
    v128.load
    v128.xor)
)

when run this yields:

$ cargo run -- --enable-simd foo.wat
    Finished dev [unoptimized + debuginfo] target(s) in 0.20s
     Running `target/debug/wasmtime --enable-simd foo.wat`
Error: failed to run main module `foo.wat`

Caused by:
    0: failed to invoke command default
    1: wasm trap: out of bounds memory access
       wasm backtrace:
           0:   0x4b - <unknown>!<wasm function 1>
           1:   0x2d - <unknown>!<wasm function 0>

It looks like in the disassembly pxor is being used with a memory operand, but presumably that memory operand needs to be aligned and the instruction is trapping otherwise?

cc @abrown, @jlb6740

@alexcrichton alexcrichton added bug Incorrect behavior in the current implementation that needs fixing wasm-proposal:simd Issues related to the WebAssembly SIMD proposal labels May 27, 2021
@abrown abrown self-assigned this Jun 17, 2021
@abrown
Copy link
Contributor

abrown commented Jul 21, 2021

presumably that memory operand needs to be aligned and the instruction is trapping otherwise

I can confirm this is the case. When I run cargo run -p cranelift-tools -- wasm --set="enable_simd=true" --target x86_64 -dDv scratch.wat with the code above I see the following:

Disassembly of 48 bytes:
   0:   55                      push    rbp
   1:   48 89 e5                mov     rbp, rsp
   4:   f3 0f 6f 05 14 00 00 00 movdqu  xmm0, xmmword ptr [rip + 0x14]
   c:   48 8b 37                mov     rsi, qword ptr [rdi]
   f:   66 0f ef 46 01          pxor    xmm0, xmmword ptr [rsi + 1]
  14:   48 89 ec                mov     rsp, rbp
  17:   5d                      pop     rbp
  18:   c3                      ret
  19:   00 00                   add     byte ptr [rax], al
  ...

There are two problems here:

  • an optimization issue: MOVDQU, the unaligned move for double-quadwords (128-bit), is used to materialize the constant at offset 19 (relative 14) into an XMM register. This actually should just be lowered to a PXOR and I'm not entirely sure why the x64 backend is not doing this
  • an alignment issue, causing the crash: PXOR is using load-coalescing for its second operand, xmmword ptr [rsi + 1], instead of a MOVDQU + PXOR sequence. This points out that load-coalescing is being over-eager: we should only be load coalescing 128-bit values if we know the load offset is aligned, perhaps using Wasm's memarg alignment.

abrown added a commit to abrown/wasmtime that referenced this issue Jul 21, 2021
Fixes bytecodealliance#2943, though not as optimally as may be desired. With x64 SIMD
instructions, the memory operand must be aligned--this change adds that
check. There are cases, however, where we can do better--see bytecodealliance#3106.
abrown added a commit that referenced this issue Jul 26, 2021
Fixes #2943, though not as optimally as may be desired. With x64 SIMD
instructions, the memory operand must be aligned--this change adds that
check. There are cases, however, where we can do better--see #3106.
wgwoods pushed a commit to wgwoods/wasmtime that referenced this issue Jul 29, 2021
Fixes bytecodealliance#2943, though not as optimally as may be desired. With x64 SIMD
instructions, the memory operand must be aligned--this change adds that
check. There are cases, however, where we can do better--see bytecodealliance#3106.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing wasm-proposal:simd Issues related to the WebAssembly SIMD proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants