-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
+279
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
cfallin
approved these changes
Feb 12, 2024
There was a problem hiding this 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...
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
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.
github-merge-queue bot
pushed a commit
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 #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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:panicking with:
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.