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

Lowering i32x4.splat + f64x2.convert_low_i32x4 results in panic #8084

Closed
ShinWonho opened this issue Mar 12, 2024 · 5 comments
Closed

Lowering i32x4.splat + f64x2.convert_low_i32x4 results in panic #8084

ShinWonho opened this issue Mar 12, 2024 · 5 comments
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift:area:aarch64 Issues related to AArch64 backend. isle Related to the ISLE domain-specific language

Comments

@ShinWonho
Copy link

Test Case

;; unsigned.wat
(module
  (func (export "foo") (result v128)
    (i32.const 0)
    (i32x4.splat)
    (f64x2.convert_low_i32x4_u)
  )
)
;; signed.wat
(module
  (func (export "foo") (result v128)
    (i32.const 0)
    (i32x4.splat)
    (f64x2.convert_low_i32x4_s)
  )
)

Steps to Reproduce

wasmtime unsigned.wat
wasmtime signed.wat

Expected Results

Terminate without any errors

Actual Results

thread '<unnamed>' panicked at cranelift/codegen/src/machinst/lower.rs:766:21:
should be implemented in ISLE: inst = `v6 = fcvt_from_uint.f64x2 v13  ; v13 = const0`, type = `Some(types::F64X2)`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
zsh: abort      wasmtime unsigned.wat
thread '<unnamed>' panicked at cranelift/codegen/src/machinst/lower.rs:766:21:
should be implemented in ISLE: inst = `v6 = fcvt_from_sint.f64x2 v13  ; v13 = const0`, type = `Some(types::F64X2)`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
zsh: abort      wasmtime signed.wat

Versions and Environment

Wasmtime version or commit: wasmtime-cli 18.0.2 (90db6e9 2024-02-28)

Operating system & Architecture:

  1. Server
  • OS: Ubuntu 18.04.3 LTS (Bionic Beaver)
  • Arc: x86_64
  1. IMac
  • OS: Monterey 12.6.2
  • Arc: x86_64

Extra Info

It is not reproduced in

  1. M1 MacBook Air
  • OS: Sonoma 14.3.1
  • Arc: Arm64
  1. MacBook Pro
  • Monterey 12.6.3
  • Arc: x86_64
@ShinWonho ShinWonho added the bug Incorrect behavior in the current implementation that needs fixing label Mar 12, 2024
@fitzgen
Copy link
Member

fitzgen commented Mar 12, 2024

Thanks for the bug report!

@fitzgen fitzgen added cranelift:area:aarch64 Issues related to AArch64 backend. isle Related to the ISLE domain-specific language labels Mar 12, 2024
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "isle"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton
Copy link
Member

This was "introduced" in #7859 which was backported to the 18.0.0 release in #7878. I say "introduced" here because this was sort of always a bug and that PR just happened to expose it, it's not directly caused by that PR per se.

Fuzzing later found this bug which resulted in #7915 as a band-aid fix for the issue followed up with #7919 for a "real fix" for at least the unsigned case. We'd forgotten about the signed case though (oops!) so I can work on that.

I'll note though that the band-aid of #7915 means that this issue doesn't reproduce as-is on main today.

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Mar 12, 2024
This instruction was previously unimplemented in Cranelift as pointed
out in bytecodealliance#8084. While a fallback for `fcvt_from_uint` was implemented
in bytecodealliance#7919 I forgot to do the same for the signed version. This commit
adds a fallback that decomposes the input into two scalars and converts
each individually, then reassembling the result.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Mar 12, 2024
This instruction was previously unimplemented in Cranelift as pointed
out in bytecodealliance#8084. While a fallback for `fcvt_from_uint` was implemented
in bytecodealliance#7919 I forgot to do the same for the signed version. This commit
adds a fallback that decomposes the input into two scalars and converts
each individually, then reassembling the result.
@alexcrichton
Copy link
Member

I posted #8099 for the missing signed conversion, but AFAIK that's just "good to have" and isn't require for correct operation on main.

The release-19.0.0 branch has all the various fixes there as well, so the only question remaining I think is effectively whether or not we want to backport #7915 to the 18.0.x branch.

github-merge-queue bot pushed a commit that referenced this issue Mar 12, 2024
This instruction was previously unimplemented in Cranelift as pointed
out in #8084. While a fallback for `fcvt_from_uint` was implemented
in #7919 I forgot to do the same for the signed version. This commit
adds a fallback that decomposes the input into two scalars and converts
each individually, then reassembling the result.
@alexcrichton
Copy link
Member

I think we're more-or-less done with this now so I'm going to close this.

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 cranelift:area:aarch64 Issues related to AArch64 backend. isle Related to the ISLE domain-specific language
Projects
None yet
Development

No branches or pull requests

3 participants