libm: fix acosh & acoshf for negative inputs #1070
Conversation
| // FIXME(#939): this should not be skipped, there is a bug in our implementationi. | ||
| if ctx.base_name == BaseName::FmaximumNum | ||
| && ctx.basis == CheckBasis::Mpfr | ||
| && ((input.0.is_nan() && actual.is_nan() && expected.is_nan()) || input.1.is_nan()) | ||
| { | ||
| return XFAIL_NOCHECK; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is unrelated to acoshf, but was just a leftover from #939 which is already merged.
quaternic
changed the title
With the json target specification format destabilized in rust-lang/rust#150151, `-Zjson-target-spec` is needed for custom targets. This should resolve the CI failures seen in #1070
This comment has been minimized.
This comment has been minimized.
libm/src/math/acosh.rs
Outdated
| return log1p(x - 1.0 + sqrt((x - 1.0) * (x - 1.0) + 2.0 * (x - 1.0))); | ||
| let x_1 = x - 1.0; | ||
| log1p(x_1 + sqrt(x_1 * x_1 + 2.0 * x_1)) | ||
| } else if u < 0x3ff + 26 { |
There was a problem hiding this comment.
Unfortunately the nonessential changes ended up hiding the actual bug fix in the diff, which is simply replacing
if e < 0x3ff + 26 {with
if u < 0x3ff + 26 {The difference is that u hasn't had the sign bit cleared, so negative inputs no longer take that branch. The fix for acoshf is equivalent.
|
To clarify; musl fixed their acoshf but not acosh? And this PR fixes both for us? |
libm/src/math/acosh.rs
Outdated
| pub fn acosh(x: f64) -> f64 { | ||
| let u = x.to_bits(); | ||
| let e = ((u >> 52) as usize) & 0x7ff; | ||
| let u = x.to_bits() >> 52; | ||
| let e = u & 0x7ff; |
There was a problem hiding this comment.
I'm assuming the as usize cast gave slightly better codegen on 32-bit systems, was it problematic?
There was a problem hiding this comment.
Oh, that would explain the cast. I didn't consider that. Even casting to u16 should work just as well, and I would hope the optimizations don't need that. (Tested on godbolt: Some insignificant differences on 32-bit.)
There was a problem hiding this comment.
Tested on godbolt: Some insignificant differences on 32-bit.
Do you mean 32-bit codegen didn't regress? If so then I suppose it's fine to keep. Otherwise, .ex() can be used
compiler-builtins/libm/src/math/support/float_traits.rs
Lines 138 to 141 in 0e5f78c
There was a problem hiding this comment.
Rewriting in terms of .ex() regressed icounts by ~7% because the semantically clear x.ex() < FOO && x.is_sign_positive() didn't optimize that well. The current version just does unsigned comparisons on ux = x.to_bits(), and seems to be good enough.
Yes, exactly. |
libm/src/math/acosh.rs
Outdated
| } else if u < 0x3ff + 26 { | ||
| /* 2 <= x < 0x1p26 */ | ||
| log(2.0 * x - 1.0 / (x + sqrt(x * x - 1.0))) |
There was a problem hiding this comment.
u is usually the value of x.to_bits() so I'd slightly prefer to just use e here if that works, or give it a different name if not.
quaternic
force-pushed
the
acosh-bug
branch
2 times, most recently
from
33278b2 to
e78a7e6
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. |
This was left over from f6a23a7 ("fmaximum,fminimum: Fix incorrect result and add tests"). [ added context to body - Trevor ]
The acosh functions were incorrectly returning finite values for some negative inputs (should be NaN for any `x < 1.0`) The bug was inherited when originally ported from musl, and this patch follows their fix for single-precision acoshf in [1]. A similar fix is applied to acosh, though musl still has an incorrect implementation requiring tests against that basis to be skipped. [1]: https://git.musl-libc.org/cgit/musl/commit/?id=c4c38e6364323b6d83ba3428464e19987b981d7a [ added context to message - Trevor ]
tgross35
left a comment
tgross35
left a comment
There was a problem hiding this comment.
Thanks for the updates, LGTM.
I squashed the last two commits since they're related and added some more context to the commit messages.
Thanks! |
With the json target specification format destabilized in rust-lang#150151, `-Zjson-target-spec` is needed for custom targets. This should resolve the CI failures seen in rust-lang/compiler-builtins#1070
3 participants
The acosh-functions were incorrectly returning finite values for some negative inputs (should be NaN for any
x < 1.0)The bug was inherited when originally ported from musl, and this patch follows their fix for single-precision acoshf in https://git.musl-libc.org/cgit/musl/commit/?id=c4c38e6364323b6d83ba3428464e19987b981d7a