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

Constant propagate int-to-float conversions #7915

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

alexcrichton
Copy link
Member

This commit is born out of a fuzz bug on x64 that was discovered recently. Today, on main, and in the 17.0.1 release Wasmtime will panic when compiling this wasm module for x64:

(module
  (func (result v128)
    i32.const 0
    i32x4.splat
    f64x2.convert_low_i32x4_u))

panicking with:

thread '<unnamed>' panicked at /home/alex/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cranelift-codegen-0.104.1/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

Bisections points to the "cause" of this regression as #7859 which more-or-less means that this has always been an issue and that PR just happened to expose the issue. What's happening here is that egraph optimizations are turning the IR into a form that the x64 backend can't codegen. Namely there's no general purpose lowering of i64x2 being converted to f64x2. The Wasm frontend never produces this but the optimizations internally end up producing this.

Notably here the result of this function is constant and what's happening is that a convert-of-a-splat is happening. In lieu of adding the full general lowering to x64 (which is perhaps overdue since this is the second or third time this panic has been triggered) I've opted to add constant propagation optimizations for int-to-float conversions. These are all based on the Rust as operator which has the same semantics as Cranelift. This is enough to fix the issue here for the time being.

This commit is born out of a fuzz bug on x64 that was discovered recently.
Today, on `main`, and in the 17.0.1 release Wasmtime will panic when compiling
this wasm module for x64:

    (module
      (func (result v128)
        i32.const 0
        i32x4.splat
        f64x2.convert_low_i32x4_u))

panicking with:

    thread '<unnamed>' panicked at /home/alex/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cranelift-codegen-0.104.1/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

Bisections points to the "cause" of this regression as bytecodealliance#7859 which
more-or-less means that this has always been an issue and that PR just
happened to expose the issue. What's happening here is that egraph
optimizations are turning the IR into a form that the x64 backend can't
codegen. Namely there's no general purpose lowering of i64x2 being
converted to f64x2. The Wasm frontend never produces this but the
optimizations internally end up producing this.

Notably here the result of this function is constant and what's
happening is that a convert-of-a-splat is happening. In lieu of adding
the full general lowering to x64 (which is perhaps overdue since this is
the second or third time this panic has been triggered) I've opted to
add constant propagation optimizations for int-to-float conversions.
These are all based on the Rust `as` operator which has the same
semantics as Cranelift. This is enough to fix the issue here for the
time being.
@alexcrichton alexcrichton requested review from a team as code owners February 12, 2024 17:06
@alexcrichton alexcrichton requested review from cfallin and pchickey and removed request for a team February 12, 2024 17:06
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks! I agree we should implement the general op eventually but this is a fine band-aid for now...

@cfallin cfallin enabled auto-merge February 12, 2024 17:10
@cfallin cfallin added this pull request to the merge queue Feb 12, 2024
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language labels Feb 12, 2024
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "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.

Merged via the queue into bytecodealliance:main with commit 8a2d9bc Feb 12, 2024
19 checks passed
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Feb 12, 2024
This commit adds a general purpose lowering for the `fcvt_from_uint`
instruction in addition to the previously specialized lowering for what
the wasm frontend produces. This is unlikely to get triggered much from
the wasm frontend except when intermediate optimizations change the
shape of code. The goal of this commit is to address issues such as
those identified in bytecodealliance#7915 and historically by ensuring that there's a
lowering for the instruction for all input types instead of trying to
massage the input into the right form.

This instruction lowering was crafted by studying LLVM's output and I've
put commentary to the best of my ability as to what's going on.
@alexcrichton alexcrichton deleted the fix-x64-panic branch February 12, 2024 18:21
github-merge-queue bot pushed a commit that referenced this pull request Feb 12, 2024

Verified

This commit was signed with the committer’s verified signature. The key has expired.
hiyuki2578 Shota Tsunehiro
This commit adds a general purpose lowering for the `fcvt_from_uint`
instruction in addition to the previously specialized lowering for what
the wasm frontend produces. This is unlikely to get triggered much from
the wasm frontend except when intermediate optimizations change the
shape of code. The goal of this commit is to address issues such as
those identified in #7915 and historically by ensuring that there's a
lowering for the instruction for all input types instead of trying to
massage the input into the right form.

This instruction lowering was crafted by studying LLVM's output and I've
put commentary to the best of my ability as to what's going on.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Mar 12, 2024

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
…#7915)"

This reverts commit 8a2d9bc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants