-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
Add mul_add_relaxed methods for floating-point types #151793
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
Open
landsharkiest
wants to merge
1
commit into
rust-lang:main
Choose a base branch
from
landsharkiest:float_mul_add_relaxed-#151770
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,76 @@ | ||||||
| //@ run-pass | ||||||
|
landsharkiest marked this conversation as resolved.
|
||||||
| //@ compile-flags: -O | ||||||
|
|
||||||
| // Check that `mul_add_relaxed` returns either the fused result (one rounding) | ||||||
| // or the unfused result (two roundings), including with optimizations enabled | ||||||
| // where the operation may be const-folded or lowered to a fused instruction. | ||||||
|
|
||||||
| #![feature(float_mul_add_relaxed)] | ||||||
| #![feature(f16)] | ||||||
| #![feature(f128)] | ||||||
| #![feature(cfg_target_has_reliable_f16_f128)] | ||||||
| // `f16`/`f128` go unused on targets without reliable f16/f128 math, where the | ||||||
| // gated test functions below are compiled out. | ||||||
| #![allow(unused_features)] | ||||||
| // `target_has_reliable_*` are not "known" configs since they are unstable. | ||||||
| #![expect(unexpected_cfgs)] | ||||||
|
|
||||||
| use std::hint::black_box; | ||||||
|
|
||||||
| fn main() { | ||||||
| test_f32(); | ||||||
| test_f64(); | ||||||
| #[cfg(target_has_reliable_f16_math)] | ||||||
| test_f16(); | ||||||
| #[cfg(target_has_reliable_f128_math)] | ||||||
| test_f128(); | ||||||
| } | ||||||
|
|
||||||
| fn test_f32() { | ||||||
| // Exactly representable results are the same whether or not the | ||||||
| // operation is fused. | ||||||
| assert_eq!(black_box(2.0_f32).mul_add_relaxed(3.0, 4.0), 10.0); | ||||||
| assert_eq!(black_box(1.0_f32).mul_add_relaxed(1.0, 1.0), 2.0); | ||||||
|
|
||||||
| // `0.1 * 0.1` is inexact, so the fused (one rounding) and unfused (two | ||||||
| // roundings) results differ; either is allowed. | ||||||
| let r = black_box(0.1_f32).mul_add_relaxed(0.1, -0.01); | ||||||
| assert!(r == 5.2154064e-10 || r == 9.313226e-10); | ||||||
|
|
||||||
| // Edge cases behave like `a * b + c` regardless of fusion. | ||||||
| assert!(black_box(f32::NAN).mul_add_relaxed(1.0, 1.0).is_nan()); | ||||||
| assert_eq!(black_box(f32::INFINITY).mul_add_relaxed(2.0, 1.0), f32::INFINITY); | ||||||
| assert!(black_box(0.0_f32).mul_add_relaxed(f32::INFINITY, 1.0).is_nan()); | ||||||
| } | ||||||
|
|
||||||
| fn test_f64() { | ||||||
| assert_eq!(black_box(2.0_f64).mul_add_relaxed(3.0, 4.0), 10.0); | ||||||
| assert_eq!(black_box(1.0_f64).mul_add_relaxed(1.0, 1.0), 2.0); | ||||||
|
|
||||||
| let r = black_box(0.1_f64).mul_add_relaxed(0.1, -0.01); | ||||||
| assert!(r == 9.020562075079397e-19 || r == 1.734723475976807e-18); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can now make this
Suggested change
and that should not just print that it failed, but also show the value that didn't match. |
||||||
|
|
||||||
| assert!(black_box(f64::NAN).mul_add_relaxed(1.0, 1.0).is_nan()); | ||||||
| assert_eq!(black_box(f64::INFINITY).mul_add_relaxed(2.0, 1.0), f64::INFINITY); | ||||||
| assert!(black_box(0.0_f64).mul_add_relaxed(f64::INFINITY, 1.0).is_nan()); | ||||||
| } | ||||||
|
|
||||||
| #[cfg(target_has_reliable_f16_math)] | ||||||
| fn test_f16() { | ||||||
| assert_eq!(black_box(2.0_f16).mul_add_relaxed(3.0, 4.0), 10.0); | ||||||
| assert_eq!(black_box(1.0_f16).mul_add_relaxed(1.0, 1.0), 2.0); | ||||||
|
|
||||||
| assert!(black_box(f16::NAN).mul_add_relaxed(1.0, 1.0).is_nan()); | ||||||
| assert_eq!(black_box(f16::INFINITY).mul_add_relaxed(2.0, 1.0), f16::INFINITY); | ||||||
| assert!(black_box(0.0_f16).mul_add_relaxed(f16::INFINITY, 1.0).is_nan()); | ||||||
| } | ||||||
|
|
||||||
| #[cfg(target_has_reliable_f128_math)] | ||||||
| fn test_f128() { | ||||||
| assert_eq!(black_box(2.0_f128).mul_add_relaxed(3.0, 4.0), 10.0); | ||||||
| assert_eq!(black_box(1.0_f128).mul_add_relaxed(1.0, 1.0), 2.0); | ||||||
|
|
||||||
| assert!(black_box(f128::NAN).mul_add_relaxed(1.0, 1.0).is_nan()); | ||||||
| assert_eq!(black_box(f128::INFINITY).mul_add_relaxed(2.0, 1.0), f128::INFINITY); | ||||||
| assert!(black_box(0.0_f128).mul_add_relaxed(f128::INFINITY, 1.0).is_nan()); | ||||||
| } | ||||||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this seems suspicious.
I suspect this might be Claude-generated though, I don't see the same pattern repeated elsewhere (e.g., in f16 just below). @landsharkiest, is there some difference between f16/f128 miri behavior?
If this is needed I'll poke Miri folks to figure out how we can avoid it being needed, in the meantime I would drop miri from the cfg here. The right way to express this implication is for Miri to set target_has_reliable (if it's reliable) IMO.
View changes since the review
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target_has_reliable_f128is always set in Miri, buttarget_has_reliable_f128_mathis not since that also guards some operations Miri does not support.Unfortunately
target_has_reliable_f128_mathguards a wide range of things, from stuff Miri can trivially support via softfloats (like mul_add) to things Miri cannot support because f128 isn't reliable on many of our targets (like pow, sin, log, etc).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, interesting. Maybe that suggests we need f128_simple_math? :)
Does the f16 situation differ for some reason (e.g., possible to add tables for literally all values?)?
If we think this is the right thing to do happy to r+ as-is though.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For f16 its similar. All the hosts that we build Miri on support it well enough that we can at least link pow, sin and friends without causing issues, and we set
target_has_reliable_f16_mathif the host has that flag. But actually for operations that we implement via softfloats we'd wanttarget_has_reliable_f16_mathto be unconditionally true, it's only the transcendental operations where hosts matter.However it's probably not worth splitting up the cfg flag just for Miri.
Tons of existing tests have
cfg(any(miri, target_has_reliable_f128_math))so the new one here is consistent.For f16 tons of tests actually use
#[cfg(target_has_reliable_f16)] {which contradicts the intent of that flag as it was explained to me... 🤷 .Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also FWIW both f16 and f128 guard e.g. the tests for
recip(1.0 / self) with thehas_reliableflag, not thehas_reliable_mathflag, despite @tgross35 explaining to me thathas_reliableis just for the ABI and bit-level operations (maybe including abs and copysign?) andhas_reliable_mathis for anything that needs actual math, rounding, or anything like that. For other operations (e.g.max,floor), f16 useshas_reliablebut f128 useshas_reliable_math. So it's all quite inconsistent and messy I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking discussion #t-compiler > has_reliable_{f16,f128} vs has_reliable_{f16,f128}_math