Interpreter: Implement floating point conversions#4884
Merged
Conversation
github-actions
Bot
added
the
cranelift
afonso360
reviewed
Sep 12, 2022
Contributor
afonso360
left a comment
afonso360
left a comment
There was a problem hiding this comment.
Thanks for working on this! Some small nits below.
I ran this through the fuzzer and it picked up some issues:
`fcvt_to_uint.i128` traps with `IntegerOverflow`
test interpret
function %a_128(f32) -> i128 {
block0(v0: f32):
v1 = fcvt_to_uint.i128 v0
return v1
}
; run: %a_128(0.0) == 0
The equivalent test for i32 returns 0, so this should too right?
I'm going to let this run for a little bit, but I'll report back if I find anything.
As an aside fcvt_low_from_sint allows for scalar values, but I think that is probably a mistake, since the docs explicitly state: "The result type will have half the number of vector lanes as the input."
Which doesn't make a lot of sense for scalar values. But that's probably something for a different PR.
This was referenced Sep 12, 2022
Implemented the following opcodes for the interpreter: - `FcvtToUint` - `FcvtToSint` - `FcvtToUintSat` - `FcvtToSintSat` - `FcvtFromUint` - `FcvtFromSint` - `FcvtLowFromSint` - `FvpromoteLow` - `Fvdemote` Copyright (c) 2022 Arm Limited
Copyright (c) 2022 Arm Limited
dheaton-arm
force-pushed
the
interpret-fcvt
branch
from
7d99a4c to
eb75fda
Compare
afonso360
approved these changes
Sep 12, 2022
Copyright (c) 2022 Arm Limited
jameysharp
approved these changes
Sep 20, 2022
Contributor
jameysharp
left a comment
jameysharp
left a comment
There was a problem hiding this comment.
Thank you for implementing these, @dheaton-arm! And @afonso360, thanks for fuzzing and reviewing!
This was referenced Sep 20, 2022
afonso360
pushed a commit
to afonso360/wasmtime
that referenced
this pull request
Sep 21, 2022
) * Interpreter: Implement floating point conversions Implemented the following opcodes for the interpreter: - `FcvtToUint` - `FcvtToSint` - `FcvtToUintSat` - `FcvtToSintSat` - `FcvtFromUint` - `FcvtFromSint` - `FcvtLowFromSint` - `FvpromoteLow` - `Fvdemote` Copyright (c) 2022 Arm Limited * Fix `I128` bounds checks for `FcvtTo{U,S}int{_,Sat}` Copyright (c) 2022 Arm Limited * Fix broken test Copyright (c) 2022 Arm Limited
This was referenced Sep 25, 2022
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Implemented the following opcodes for the interpreter:
FcvtToUintFcvtToSintFcvtToUintSatFcvtToSintSatFcvtFromUintFcvtFromSintFcvtLowFromSintFvpromoteLowFvdemoteCopyright (c) 2022 Arm Limited