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

Floating-point literals should store bits in integers, not use host f32/f64. #189

Open
eddyb opened this issue Apr 2, 2021 · 2 comments

Comments

@eddyb
Copy link

eddyb commented Apr 2, 2021

Compilers should never rely on host floating-point support, as that can lead to host-specific behavioral subtleties (instead of behavior being deterministically tied to the target architecture/platform), and in many cases there's not enough to gain to even look into what guarantees certain host architectures/platforms may offer.

For rustc we ported LLVM's C++ APFloat library to Rust (for e.g. evaluating float operations in constant expressions), but rspirv itself only needs to store literals, not operate on the actual values (and consumers could choose how to interpret the bits of a floating-point value, any way they want).

I'm not sure how what the timeline for 0.8 is, but this would be a breaking change.

Also, it would help with #16.

cc @khyperia

@Jasper-Bekkers
Copy link
Collaborator

@eddyb do you have any examples of where this is done wrong? Is it just the constant_f32 functions and LiteralFloat32 etc?

@eddyb
Copy link
Author

eddyb commented May 5, 2021

Right, and loading code like this:

/// Decodes and returns the next SPIR-V word as a 32-bit
/// literal floating point number.
pub fn float32(&mut self) -> Result<f32> {
let val = self.word()?;
Ok(f32::from_bits(val))
}
/// Decodes and returns the next two SPIR-V words as a 64-bit
/// literal floating point number.
pub fn float64(&mut self) -> Result<f64> {
let low = u64::from(self.word()?);
let high = u64::from(self.word()?);
let val = (high << 32) | low;
Ok(f64::from_bits(val))
}

Basically the types f32 and f64 should more or less not show up in rspirv (except maybe for fmt::Debug?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants