c-b: Preserve the NaN sign bit in soft-float addition - #1239
c-b: Preserve the NaN sign bit in soft-float addition#1239HuzaifaAbdulRehman wants to merge 2 commits into
Conversation
|
Hi @tgross35, this has been open for a couple weeks with all 44 checks passing and no conflicts with main. Happy to make any changes if you'd like a different approach, just let me know. Thanks for taking a look when you get a chance. |
|
Yeah sorry, I have seen this but have been a bit backed up for reviews, it may be another week or two since we've come across some higher priority things. As a quick note, it looks like your PR description may be machine-generated; please rewrite it, all communication with humans needs to be handwritten. Also the title should be more specific, this is only touches addition (similarly, it shouldn't close the linked issue). |
HuzaifaAbdulRehman
changed the title
90bc1ef to
3e99381
Compare
HuzaifaAbdulRehman
changed the title
|
Rewrote the description by hand, made the title specific, and dropped the Closes since this only touches addition. Reworded the commit message too, I'd left the old one on there. The red i686-pc-windows-gnu job isn't mine. That toolchain went uninstallable on the 28th and other PRs are hitting it too. No rush. |
3e99381 to
4ba8007
Compare
This comment has been minimized.
This comment has been minimized.
4ba8007 to
d86ffab
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. |
| ($name:ident, $f:ty, $add:path, $one:expr, $snan:expr, $qnan:expr) => { | ||
| #[test] | ||
| fn $name() { | ||
| let one = <$f>::from_bits($one); | ||
|
|
||
| for (input_bits, expected_bits) in [($snan, $qnan), ($qnan, $qnan)] { | ||
| let input = <$f>::from_bits(input_bits); | ||
| assert_eq!($add(input, one).to_bits(), expected_bits); | ||
| assert_eq!($add(one, input).to_bits(), expected_bits); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Use these constants
compiler-builtins/libm/src/math/support/float_traits.rs
Lines 52 to 57 in 54796a7
| mod float_addsub { | ||
| use super::*; | ||
|
|
||
| macro_rules! nan_sign_test { |
There was a problem hiding this comment.
No need for a separate macro, put this into float_sum. (Fine if it's just at the end of that test function)
| assert_eq!($add(input, one).to_bits(), expected_bits); | ||
| assert_eq!($add(one, input).to_bits(), expected_bits); |
There was a problem hiding this comment.
Please do also check sub
The extra macro was just rebuilding Float's NaN constants. Those now live at the end of float_sum, and sub is checked too.
|
Moved the NaN checks into
|
| // Quiet sNaN by setting the top significand bit. `sub(x, nan)` | ||
| // flips that sign because sub is add with b's sign bit inverted. | ||
| let one = <$f as Float>::ONE; | ||
| for input in [ | ||
| <$f as Float>::SNAN, | ||
| <$f as Float>::NAN, | ||
| <$f as Float>::NEG_SNAN, | ||
| <$f as Float>::NEG_NAN, | ||
| ] { | ||
| let expected = | ||
| <$f>::from_bits(input.to_bits() | <$f as Float>::SIG_TOP_BIT); | ||
| assert_eq!($fn_add(input, one).to_bits(), expected.to_bits()); | ||
| assert_eq!($fn_add(one, input).to_bits(), expected.to_bits()); | ||
| assert_eq!($fn_sub(input, one).to_bits(), expected.to_bits()); | ||
| let flipped = | ||
| <$f>::from_bits(expected.to_bits() ^ <$f as Float>::SIGN_MASK); | ||
| assert_eq!($fn_sub(one, input).to_bits(), flipped.to_bits()); |
There was a problem hiding this comment.
May as well check the entire matrix I suppose:
let qnan = <$f>::NAN;
let snan = <$f>::SNAN;
let qsnan = <$f>::QSNAN;
let neg_qnan = <$f>::NEG_NAN;
let neg_snan = <$f>::NEG_SNAN;
let neg_qsnan = <$f>::NEG_QSNAN;
let one = <$f>::ONE;
let nan_cases = [
(qnan, qnan, qnan),
(qnan, snan, qnan),
(qnan, neg_qnan, qnan),
(qnan, neg_snan, qnan),
(qnan, one, qnan),
(snan, qnan, qsnan),
(snan, snan, qsnan),
(snan, neg_qnan, qsnan),
(snan, neg_snan, qsnan),
(snan, one, qsnan),
(neg_qnan, qnan, neg_qnan),
(neg_qnan, snan, neg_qnan),
(neg_qnan, neg_qnan, neg_qnan),
(neg_qnan, neg_snan, neg_qnan),
(neg_qnan, one, neg_qnan),
(neg_snan, qnan, neg_qsnan),
(neg_snan, snan, neg_qsnan),
(neg_snan, neg_qnan, neg_qsnan),
(neg_snan, neg_snan, neg_qsnan),
(neg_snan, one, neg_qsnan),
];
for (x, y, expected) in add_nan_cases {
// ...
}
for (x, y, expected) in sub_cases {
// ...
}QSNAN doesn't exist yet, adding it in #1314
| let expected = | ||
| <$f>::from_bits(input.to_bits() | <$f as Float>::SIG_TOP_BIT); | ||
| assert_eq!($fn_add(input, one).to_bits(), expected.to_bits()); | ||
| assert_eq!($fn_add(one, input).to_bits(), expected.to_bits()); | ||
| assert_eq!($fn_sub(input, one).to_bits(), expected.to_bits()); | ||
| let flipped = | ||
| <$f>::from_bits(expected.to_bits() ^ <$f as Float>::SIGN_MASK); | ||
| assert_eq!($fn_sub(one, input).to_bits(), flipped.to_bits()); |
3 participants
Only addition, so not closing #1188.
The NaN branch in
add.rsreturnsF::from_bits(a_abs | quiet_bit), anda_absisa_rep & abs_mask, so the sign is already gone by then.-sNaN + 1.0comes out+qNaN.mul.rs:50 and div.rs:152 already use
a_repin the same branch, so add looks like the odd one out. Usinga_repstill ORs inquiet_bit.Tests in
addsub.rscheck sNaN and qNaN in both operand positions, by bits, f16 through f128. They fail on main.Sub is
add(a, -b), soa - NaNnow flips b's sign. Left that alone, wasn't sure if it's wanted.AI disclosure: used an assistant for the investigation and a first draft of the tests.