-
Notifications
You must be signed in to change notification settings - Fork 16
add sqrt from miri + tests #28
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
cec3240
9108da8
0a204b9
b16d17f
816b4ea
b2fe989
3fc058a
f08e8be
059fc87
1deb61a
b6404fe
64387f7
562f7ef
c13103d
d0a2760
d645d5f
fa1f260
23db47a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -484,6 +484,88 @@ pub trait Float: | |
| } | ||
| } | ||
|
|
||
| fn sqrt(self) -> Self { | ||
|
ValorZard marked this conversation as resolved.
Outdated
|
||
| match self.category() { | ||
| // preserve zero sign | ||
| Category::Zero => self, | ||
| // propagate NaN | ||
| Category::NaN => self, | ||
|
Member
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. idk if it matters, but technically you're supposed to quiet signalling NaNs.
Author
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. What do you mean by that?
Member
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. If the input is a signalling NaN, then IEEE 754 requires the result to be converted to a quiet NaN. On most CPUs that means the most significant bit of the significand field is 0 for signalling NaNs and 1 for quiet NaNs. On most CPUs they quiet a NaN by setting that bit to a 1, RISC-V instead returns the canonical NaN with positive sign, the most significant significand bit set and all other significand bits cleared. However, Rust and LLVM allow input NaNs to be returned unmodified as well as a few other options -- see Rust's rules for NaNs.
Author
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. okay so i think returning self is fine then.
Member
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.
sure, go ahead. you'll probably want to change the linked part to instead have the link's url on a line by itself
Member
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. This crate otherwise does a good job of sticking to IEEE semantics and quietens sNaNs elsewhere. I think we should do the same here - it's easy enough, just use
Welcome to the world of floating point 🙃. Crash course: there are two kinds of NaN, signaling (pretty useless and extremely annoying, but part of the spec) and quiet (used pretty much everywhere; this is Rust's You can see the exceptions in Rust, via asm, or in C: |
||
| // sqrt of negative number is NaN | ||
| _ if self.is_negative() => Self::NAN, | ||
| // sqrt(∞) = ∞ | ||
| Category::Infinity => Self::INFINITY, | ||
| Category::Normal => { | ||
| // Floating point precision, excluding the integer bit | ||
| let prec = i32::try_from(Self::PRECISION).unwrap() - 1; | ||
|
|
||
| // x = 2^(exp - prec) * mant | ||
| // where mant is an integer with prec+1 bits | ||
| // mant is a u128, which should be large enough for the largest prec (112 for f128) | ||
| let mut exp = self.ilogb(); | ||
| let mut mant = self.scalbn(prec - exp).to_u128(128).value; | ||
|
|
||
| if exp % 2 != 0 { | ||
| // Make exponent even, so it can be divided by 2 | ||
| exp -= 1; | ||
| mant <<= 1; | ||
| } | ||
|
|
||
| // Bit-by-bit (base-2 digit-by-digit) sqrt of mant. | ||
| // mant is treated here as a fixed point number with prec fractional bits. | ||
| // mant will be shifted left by one bit to have an extra fractional bit, which | ||
| // will be used to determine the rounding direction. | ||
|
|
||
| // res is the truncated sqrt of mant, where one bit is added at each iteration. | ||
| let mut res = 0u128; | ||
| // rem is the remainder with the current res | ||
| // rem_i = 2^i * ((mant<<1) - res_i^2) | ||
| // starting with res = 0, rem = mant<<1 | ||
| let mut rem = mant << 1; | ||
| // s_i = 2*res_i | ||
| let mut s = 0u128; | ||
| // d is used to iterate over bits, from high to low (d_i = 2^(-i)) | ||
| let mut d = 1u128 << (prec + 1); | ||
|
|
||
| // For iteration j=i+1, we need to find largest b_j = 0 or 1 such that | ||
| // (res_i + b_j * 2^(-j))^2 <= mant<<1 | ||
| // Expanding (a + b)^2 = a^2 + b^2 + 2*a*b: | ||
| // res_i^2 + (b_j * 2^(-j))^2 + 2 * res_i * b_j * 2^(-j) <= mant<<1 | ||
| // And rearranging the terms: | ||
| // b_j^2 * 2^(-j) + 2 * res_i * b_j <= 2^j * (mant<<1 - res_i^2) | ||
| // b_j^2 * 2^(-j) + 2 * res_i * b_j <= rem_i | ||
|
|
||
| while d != 0 { | ||
| // Probe b_j^2 * 2^(-j) + 2 * res_i * b_j <= rem_i with b_j = 1: | ||
| // t = 2*res_i + 2^(-j) | ||
| let t = s + d; | ||
| if rem >= t { | ||
| // b_j should be 1, so make res_j = res_i + 2^(-j) and adjust rem | ||
| res += d; | ||
| s += d + d; | ||
| rem -= t; | ||
| } | ||
| // Adjust rem for next iteration | ||
| rem <<= 1; | ||
| // Shift iterator | ||
| d >>= 1; | ||
| } | ||
|
|
||
| // Remove extra fractional bit from result, rounding to nearest. | ||
| // If the last bit is 0, then the nearest neighbor is definitely the lower one. | ||
| // If the last bit is 1, it sounds like this may either be a tie (if there's | ||
| // infinitely many 0s after this 1), or the nearest neighbor is the upper one. | ||
| // However, since square roots are either exact or irrational, and an exact root | ||
| // would lead to the last "extra" bit being 0, we can exclude a tie in this case. | ||
| // We therefore always round up if the last bit is 1. When the last bit is 0, | ||
| // adding 1 will not do anything since the shift will discard it. | ||
| res = (res + 1) >> 1; | ||
|
|
||
| // Build resulting value with res as mantissa and exp/2 as exponent | ||
| Self::from_u128(res).value.scalbn(exp / 2 - prec) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// IEEE-754R isSignMinus: Returns true if and only if the current value is | ||
| /// negative. | ||
| /// | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ use core::cmp::Ordering; | |
| use rustc_apfloat::ieee::{BFloat, Double, Float8E4M3FN, Float8E5M2, Half, Quad, Single, X87DoubleExtended}; | ||
| use rustc_apfloat::{Category, ExpInt, IEK_INF, IEK_NAN, IEK_ZERO}; | ||
| use rustc_apfloat::{Float, FloatConvert, Round, Status}; | ||
| use std::ops::Neg; | ||
|
|
||
| // FIXME(eddyb) maybe include this in `rustc_apfloat` itself? | ||
| macro_rules! define_for_each_float_type { | ||
|
|
@@ -795,6 +796,21 @@ fn maximum() { | |
| assert!(nan.maximum(f1).to_f64().is_nan()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn sqrt() { | ||
|
Member
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. Now you're only testing
Member
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. Actually,
Member
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. Put this in |
||
| let f1 = Double::from_f64(64.); | ||
| let f2 = Double::from_f64(8.); | ||
| assert_eq!(f1.sqrt().to_f64(), f2.to_f64()); | ||
| assert_eq!(f1.sqrt().to_f64(), 64_f64.sqrt()); | ||
| assert_eq!(f2.sqrt().to_f64(), 8_f64.sqrt()); | ||
| assert_eq!(Double::INFINITY.sqrt().to_f64(), f64::INFINITY); | ||
| assert_eq!(Double::ZERO.sqrt().to_f64().total_cmp(&0.0), std::cmp::Ordering::Equal); | ||
| assert_eq!((-Double::ZERO).sqrt().to_f64().total_cmp(&-0.0), std::cmp::Ordering::Equal); | ||
| assert!(Double::from_f64(-5.0).sqrt().is_nan()); | ||
| assert!(Double::INFINITY.neg().sqrt().is_nan()); | ||
| assert!(Double::NAN.sqrt().is_nan()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn denormal() { | ||
| // Test single precision | ||
|
|
||
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.
IEEE sqrt can take rounding mode and raise the floating point exceptions, so technically this should return a
StatusAnd. I think it's fine to always round to nearest and disregard exceptions, but please document this.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.
you can check the comment i added to see if its what you want
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.
Currently:
The reasoning isn't correct: exact means correctly rounded, it doesn't mean no rounding will occur. You can have exact results with any of the four rounding modes. Instead it should just say that we don't support this.
Please also note that this is public documentation so the first line should just say what the function is before jumping into its details. Also please quote code in
`...`.