add a fallback for more f16 intrinsics #159175
Conversation
|
r? tgross35 |
|
|
f878252 to
3db7de3
Compare
This comment has been minimized.
This comment has been minimized.
|
I've added some more fallbacks based on what rust/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs Lines 425 to 436 in d62fb1c rust/compiler/rustc_codegen_gcc/src/intrinsic/mod.rs Lines 167 to 184 in d62fb1c |
This comment has been minimized.
This comment has been minimized.
3db7de3 to
c54fad0
Compare
|
I split out #159386, that one is a bit more subtle and I'm not sure what we want to do with those |
c54fad0 to
37bfe32
Compare
| pub const fn copysignf32(x: f32, y: f32) -> f32; | ||
| pub const fn copysignf32(x: f32, y: f32) -> f32 { | ||
| f32::from_bits((x.to_bits() & !f32::SIGN_MASK) | (y.to_bits() & f32::SIGN_MASK)) | ||
| } |
There was a problem hiding this comment.
@RalfJung should I remove the custom const-eval implementation (and add miri::intrinsic_fallback_is_spec) when adding a const fallback? const-eval already duplicates minimumf*, and I think it makes sense to split out sym::fabs into a variant for each float too (all backends need to retrieve the width anyway, if you look at its usage).
There was a problem hiding this comment.
fabs was merged a few months ago by @N1ark and I think she planned to merge the others as well. Just from the perspective of not having to duplicate the signatures and doc comments four times in this file, it'd be nice to merge more intrinsics... but either way please coordinate with her. I am very much not a fan of how the float intrinsics are turning this file into a mess where every change has to be done 4 times, so I think merging the different type variants is quite important.
Merging makes the fallback bodies more tricky of course. For these bitwise operations maybe a simple trait is enough to implement them generically?
Minimum has non-determinism and other complications, so that's not duplicated in const-eval, it's a genuinely different implementation. For these here... not sure, they seem sufficiently primitive to me that I think it's worth keeping the existing const-eval implementations.
There was a problem hiding this comment.
yes sorry i started doing it in #153934 and never got around to finishing it 🫠 ill pick it back up next week, @folkertdev once im done maybe rebasing this PR on that will make your life easier?
There was a problem hiding this comment.
Not really, combining the intrinsics means it's impossible to add fallbacks for just f16 (by deferring to f32). Adding those fallbacks removes nasty logic from the backends, and if the price is a to copy-paste a comment, that seems acceptable to me? Especially for simple intrinsics without complex safety/soundness requirements.
For abs and copysign we can use some trait machinery to provide a fallback for all four float types, but for functions like powf or sqrt that won't work.
There was a problem hiding this comment.
I have edited the various minimum intrinsics' docs quite a bit, so those certainly do not qualify as "simple". So I feel very strongly that we should deduplicate these comments -- but we can do that without merging the functions, by writings the docs once on the f32 version and making all the others a link.
There was a problem hiding this comment.
Yeah, fair. So then I think next steps are
- combine the docs of some of the trickier functions
- write a generic fallback for e.g.
absandcopysignwhere we can reasonably provide a fallback for all 4 float types. - simplify the backends because they won't have to handle many libcall cases any more (I made a start with that in use core fallbacks for more
f16/f128operations rustc_codegen_gcc#910), with this PR we can do more, and cranelift can also remove some logic. It's probably best to do this in their respective repos for improved CI coverage.
@N1ark are you interested in any of that? I'll be doing some of the cranelift stuff to remove another .patch there but otherwise I'm happy to leave it to you.
| #[rustc_nounwind] | ||
| pub const fn fmaf16(a: f16, b: f16, c: f16) -> f16; | ||
| pub const fn fmaf16(a: f16, b: f16, c: f16) -> f16 { | ||
| fmaf64(a as f64, b as f64, c as f64) as f16 |
There was a problem hiding this comment.
Could you add a comment that f32 isn't enough precision so we need f64? The difference isn't obvious if you don't know the context.
There was a problem hiding this comment.
Have you verified that f64 gives enough precision? I am not aware of a theorem for this case.
There was a problem hiding this comment.
Dug it up, by @beetrees of course llvm/llvm-project#128450 (comment).
There was a problem hiding this comment.
Hm, I can't really follow but good to know they looked into it. :D
This might be something one can just exhaustively test though?
37bfe32 to
91ecf62
Compare
This comment has been minimized.
This comment has been minimized.
- `floorf16` - `ceilf16` - `truncf16,` - `round_ties_even_f16` - `roundf16` - `powif16` - `sqrtf16` - `fmaf16`
91ecf62 to
da925bb
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r=tgross35 |
…, r=tgross35 add a fallback for more `f16` intrinsics tracking issue: rust-lang#116909 Add several more fallbacks for `f16` intrinsics, so that they will work on platforms without the corresponding libcalls. related - rust-lang/rustc_codegen_cranelift#1675 - rust-lang/rustc_codegen_cranelift#1674 - rust-lang#150946
Rollup of 5 pull requests Successful merges: - #159164 ([cg_ssa] Eliminate the `is_backend_{immediate,scalar_pair,ref}` methods) - #159175 (add a fallback for more `f16` intrinsics ) - #158586 (update 'allocation' docs with memory atomicity requirement) - #159390 (rustc: Avoid passing jobserver proxy around the compiler) - #159468 (Android platform support: add links to make it easier to figure out the supported API levels)
View all comments
tracking issue: #116909
Add several more fallbacks for
f16intrinsics, so that they will work on platforms without the corresponding libcalls.related