Skip to content

components: Use f{32,64} as wrapped type of Val::Float{32,64} #5480

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

Closed
lann opened this issue Dec 20, 2022 · 5 comments · Fixed by #5510
Closed

components: Use f{32,64} as wrapped type of Val::Float{32,64} #5480

lann opened this issue Dec 20, 2022 · 5 comments · Fixed by #5510
Labels
wasm-proposal:component-model Issues related to the WebAssembly Component Model proposal

Comments

@lann
Copy link
Contributor

lann commented Dec 20, 2022

Currently the wasmtime::component::Val::Float32 and ::Float64 variants wrap u32 and u64 values:

Float32(u32),
Float64(u64),

I suspect this is just an oversight as typed functions do use f32/f64. I'm happy to make this update if it makes sense.

@dicej
Copy link
Contributor

dicej commented Dec 20, 2022

For the record, this floats-as-bits representation was copied from the equivalent API for modules:

/// A 32-bit float.
///
/// Note that the raw bits of the float are stored here, and you can use
/// `f32::from_bits` to create an `f32` value.
F32(u32),
/// A 64-bit float.
///
/// Note that the raw bits of the float are stored here, and you can use
/// `f64::from_bits` to create an `f64` value.
F64(u64),
, which was added in this commit: f88e92a. I imagine that if we're going to change this API, we should also change the module API as well. Edit: As Lann pointed out, since the component model requires NaNs to be canonicalized, it may make sense to use a different representation for component values vs. core Wasm values.

@jameysharp
Copy link
Contributor

We do this in Cranelift too. I don't know if it's for the same reason that Wasmtime's API works this way, but Cranelift's reason was discussed in #2251. In short, Rust doesn't make any guarantees about the bit-representation of NaN values being preserved in floating-point types. So if you care about that, you need to keep the raw bits in an integer type until the time when you want to do actual floating-point operations on them.

@lann
Copy link
Contributor Author

lann commented Dec 20, 2022

@jameysharp Thanks for that context. The component model requires canonicalization of NaNs so it seems like it makes sense to diverge from the copied API here.

@alexcrichton
Copy link
Member

I agree that while this matches the core wasm API it can be different for hte ocmponent model due to all NaN's being canonicalized. @lann would you be up for PR-ing this change?

@alexcrichton alexcrichton added the wasm-proposal:component-model Issues related to the WebAssembly Component Model proposal label Jan 3, 2023
@lann
Copy link
Contributor Author

lann commented Jan 3, 2023

Sure, I can take it.

lann added a commit to lann/wasmtime that referenced this issue Jan 3, 2023
…2,64}`

The definitions of `wasmtime::component::Val::Float{32,64}` mirrored
`wasmtime::Val::F{32,64}` in using `u{32,64}` as their wrapped types.
This is necessary for the core wasm `f32`/`f64` types because Rust's
floating-point types don't guarantee the preservation of NaN bit
representations. The component model `float32`/`float64` on the other
hand require NaN canonicalization, so we can use the more natural
Rust `f{32,64}` instead.

Closes bytecodealliance#5480
lann added a commit to lann/wasmtime that referenced this issue Jan 3, 2023
…2,64}`

The definitions of `wasmtime::component::Val::Float{32,64}` mirrored
`wasmtime::Val::F{32,64}` in using `u{32,64}` as their wrapped types.
This is necessary for the core wasm `f32`/`f64` types because Rust's
floating-point types don't guarantee the preservation of NaN bit
representations. The component model `float32`/`float64` on the other
hand require NaN canonicalization, so we can use the more natural
Rust `f{32,64}` instead.

Closes bytecodealliance#5480
lann added a commit to lann/wasmtime that referenced this issue Jan 3, 2023
The definitions of `wasmtime::component::Val::Float{32,64}` mirrored
`wasmtime::Val::F{32,64}` in using `u{32,64}` as their wrapped types.
This is necessary for the core wasm `f32`/`f64` types because Rust's
floating-point types don't guarantee the preservation of NaN bit
representations. The component model `float32`/`float64` on the other
hand require NaN canonicalization, so we can use the more natural
Rust `f{32,64}` instead.

Closes bytecodealliance#5480
lann added a commit to lann/wasmtime that referenced this issue Jan 3, 2023
The definitions of `wasmtime::component::Val::Float{32,64}` mirrored
`wasmtime::Val::F{32,64}` by using integers as their wrapped types,
storing the bit representation of their floating point values.
This was necessary for the core Wasm `f32`/`f64` types because Rust
floats don't have guaranteed NaN bit representations.

The component model `float32`/`float64` types require NaN
canonicalization, so we can use normal Rust `f{32,64}` instead.

Closes bytecodealliance#5480
alexcrichton pushed a commit that referenced this issue Jan 3, 2023
The definitions of `wasmtime::component::Val::Float{32,64}` mirrored
`wasmtime::Val::F{32,64}` by using integers as their wrapped types,
storing the bit representation of their floating point values.
This was necessary for the core Wasm `f32`/`f64` types because Rust
floats don't have guaranteed NaN bit representations.

The component model `float32`/`float64` types require NaN
canonicalization, so we can use normal Rust `f{32,64}` instead.

Closes #5480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasm-proposal:component-model Issues related to the WebAssembly Component Model proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants