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

Investigate adding fast-math functions #12

Closed
Jasper-Bekkers opened this issue Aug 19, 2020 · 8 comments
Closed

Investigate adding fast-math functions #12

Jasper-Bekkers opened this issue Aug 19, 2020 · 8 comments
Labels
c: spirv-std Issues specific to the spirv-std crate t: enhancement A new feature or improvement to an existing one.

Comments

@Jasper-Bekkers
Copy link
Contributor

Relevant: rust-lang/rust#21690

My current thinking around this is that we probably shouldn't even do this.

However, if we do this, I'm kind of partial to having something like unsafe fn sqrt_approx() implemented on floats, that one needs to call explicitly within an unsafe block. As discussed in that issue, there are quite a few problems with the existing LLVM backend, it's optimizations etc. Many of which we don't need to care about because we're rolling our own. Having said that, SPIR-V and especially Vulkan have their own set of restrictions around floats: https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#spirvenv-precision-operation

@repi
Copy link
Contributor

repi commented Aug 19, 2020

Why would it be an unsafe function?

@Jasper-Bekkers
Copy link
Contributor Author

For an LLVM backend it would 100% need to be unsafe because LLVM tags all float operations and some of those tags can lead to unsafe optimizations being applies later on (eg. nnan tag can lead to incorrect code removal because it may/may not have incorrectly assumed that nans can not occur). That's highlighted in the rust thread.

For our backend, we kind of get to choose because we most likely won't be doing any fast-math style optimizations ourselves; however keeping with the rust spirit I'd like to by default have all the f32 operations be as safe as possible with opt in approximations. Whether we should tag them as unsafe or or not might be a bit more of a moot point, but I think it's the right call. Especially if we might consider this holistically and as a fix to that linked issue (which would imply it running on LLVM).

@Jasper-Bekkers
Copy link
Contributor Author

@hanna-kruppe was much better at explaining this: rust-lang/rust#21690 (comment)

@repi
Copy link
Contributor

repi commented Aug 19, 2020

I see, well as long as one can wrap it into safe opinionated functions ourselves for our shader code, the lower levels could be unsafe because of technicalities.

A goal of this project has to also be that it is quite ergonomic ultimately to write high-level shading code in Rust, if it is not, then it wouldn't work for our use cases. And unsafe code is far from ergonomic, but layering safe/unsafe code as one does in Rust should hopefully be able to resolve it

@Jasper-Bekkers
Copy link
Contributor Author

A goal of this project has to also be that it is quite ergonomic ultimately to write high-level shading code in Rust, if it is not, then it wouldn't work for our use cases. And unsafe code is far from ergonomic, but layering safe/unsafe code as one does in Rust should hopefully be able to resolve it

My expectation would be that 90% of the time you'd call the regular, safe .sqrt() that already lives in f32. Having the unsafe/fastmath stuff be unergonomic would be Fine By Me but we can discuss that of course / or like you say, wrap it in a "safe" layer.

@repi
Copy link
Contributor

repi commented Aug 19, 2020

One can probably find a solution, shaders are almost always built with very aggressive fastmath together with ergonomic shortcuts. At the cost of explicitness

@Jasper-Bekkers
Copy link
Contributor Author

Ah rust has some of the fast-math ones in std::intrinsics::: https://doc.rust-lang.org/std/intrinsics/fn.fadd_fast.html

@XAMPPRocky XAMPPRocky added c: spirv-std Issues specific to the spirv-std crate t: enhancement A new feature or improvement to an existing one. labels Oct 19, 2020
@khyperia
Copy link
Contributor

khyperia commented Apr 1, 2021

This probably isn't on our roadmap any time soon, so closing this until someone has a concrete need for it with opinions and desires we can design the feature around.

@khyperia khyperia closed this as completed Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: spirv-std Issues specific to the spirv-std crate t: enhancement A new feature or improvement to an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants