Skip to content

use libm for acosh and asinh for f16, f32, and f64#154051

Open
malezjaa wants to merge 1 commit intorust-lang:mainfrom
malezjaa:approximations-acosh-and-asinh
Open

use libm for acosh and asinh for f16, f32, and f64#154051
malezjaa wants to merge 1 commit intorust-lang:mainfrom
malezjaa:approximations-acosh-and-asinh

Conversation

@malezjaa
Copy link
Copy Markdown
Contributor

@malezjaa malezjaa commented Mar 18, 2026

View all comments

Fixes #153878

Uses libm for acosh and asinh for f16, f32, and f64.
I didn't change impl for f128 as i couldn't find existing function for these in libm

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 18, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 18, 2026

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @ChrisDenton, libs
  • @ChrisDenton, libs expanded to 8 candidates
  • Random selection from Mark-Simulacrum, joboet

@reddevilmidzy
Copy link
Copy Markdown
Member

Hello @malezjaa,
Next time you contribute, please check if someone has been assigned to the issue before contributing.

@malezjaa
Copy link
Copy Markdown
Contributor Author

I'm really sorry about that. I should've checked If anyone was already assigned to this issue. I'll do that from now on.

@rust-bors

This comment has been minimized.

@malezjaa malezjaa force-pushed the approximations-acosh-and-asinh branch from 4a67c1e to 90a7256 Compare March 21, 2026 09:01
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 21, 2026

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.

@tgross35
Copy link
Copy Markdown
Contributor

Happy to review this or alternatives, unless you have a preference @joboet

r? tgross35

@rustbot rustbot assigned tgross35 and unassigned joboet Mar 26, 2026
@tgross35
Copy link
Copy Markdown
Contributor

this fixes acosh and asinh for large float values using the approximation from libm

Can we just call libm via the cmath moule instead, rather than having the same logic here? Like we do for acos and similar.

Can you also clarify what approximations you are referencing? This isn't what is in https://github.com/rust-lang/compiler-builtins/blob/644346f0541f74fc425070f8e0712f2dac898e11/libm/src/math/acosh.rs.

@malezjaa malezjaa changed the title return approximations for large float values in acosh and asinh use libm for acosh and asinh for f16, f32, and f64 Mar 28, 2026
@malezjaa malezjaa marked this pull request as draft March 28, 2026 16:41
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2026
@malezjaa malezjaa force-pushed the approximations-acosh-and-asinh branch from 3a210bb to 538e681 Compare March 28, 2026 17:02
@malezjaa
Copy link
Copy Markdown
Contributor Author

Oh yeah that makes sense. I'll update the PR title and description to show that we're now using libm for these functions.
For f128 I kept the existing approximation since I couldn't find the f128 version in libm

@malezjaa malezjaa marked this pull request as ready for review March 28, 2026 17:05
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 28, 2026
@rust-log-analyzer

This comment has been minimized.

@reddevilmidzy
Copy link
Copy Markdown
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 31, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@malezjaa malezjaa force-pushed the approximations-acosh-and-asinh branch from 538e681 to abd315c Compare March 31, 2026 06:08
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 31, 2026

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Mar 31, 2026

Oh wait, this was for the i686-pc-windows-msvc target. Not sure why that would matter, but I can reproduce the problem locally with --target i686-pc-windows-msvc.

@RalfJung
Copy link
Copy Markdown
Member

RalfJung commented Mar 31, 2026

It probably has to do with this:

// On i686-pc-windows-msvc , these functions are implemented by calling the `f64` version,
// which means the little rounding errors Miri introduces are discarded by the cast down to
// `f32`. Just skip the test for them.
if !cfg!(all(target_os = "windows", target_env = "msvc", target_arch = "x86")) {

You can move the new cases into that if as well (after checking that the comment indeed applies to the two new functions as well).

@malezjaa
Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 31, 2026
@tgross35
Copy link
Copy Markdown
Contributor

I didn't change impl for f128 as i couldn't find existing function for these in libm

These are supported in glibc, which is pretty much the only libc to support f128 math for now, cfg(target_has_reliable_f128_math) accounts for this. So they are fine to do here as well.

@malezjaa malezjaa force-pushed the approximations-acosh-and-asinh branch 2 times, most recently from 2332619 to 30e7816 Compare March 31, 2026 21:29
Copy link
Copy Markdown
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'll merge after CI passes

View changes since this review

@rust-log-analyzer

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, I usually use Julia for this quick arbitrary-precision number crunching. Set to MANTISSA_DIGITS then test with MAX.

julia> setprecision(113)
113

julia> x=parse(BigFloat, "1.18973149535723176508575932662800702e+4932")
1.18973149535723176508575932662800702e+4932

julia> asinh(x)
11357.216553474703894801348310092223

julia> asinh(big(1.7976931348623157e+308))
710.475860073943942041640622032115338

julia> setprecision(53)
53

julia> asinh(big(1.7976931348623157e+308))
710.47586007394398

julia> setprecision(24)
24

julia> asinh(big(3.40282347e+38))
89.4159851

julia> setprecision(11)
11

julia> asinh(big(6.5504e+4))
11.781

View changes since this review

@malezjaa malezjaa force-pushed the approximations-acosh-and-asinh branch from 30e7816 to e83e484 Compare April 1, 2026 13:04
@malezjaa
Copy link
Copy Markdown
Contributor Author

malezjaa commented Apr 1, 2026

Nice. Thank you for the correct values :D

Copy link
Copy Markdown
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 3, 2026

📌 Commit e83e484 has been approved by tgross35

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2026
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 3, 2026
…asinh, r=tgross35

use libm for acosh and asinh for f16, f32, and f64

Fixes rust-lang#153878

Uses libm for `acosh` and `asinh` for `f16`, `f32`, and `f64`.
I didn't change impl for f128 as i couldn't find existing function for these in libm
rust-bors bot pushed a commit that referenced this pull request Apr 3, 2026
Rollup of 5 pull requests

Successful merges:

 - #153286 (various fixes for scalable vectors)
 - #153592 (Add  `min_adt_const_params` gate)
 - #154051 (use libm for acosh and asinh for f16, f32, and f64)
 - #154743 (Remove an unused `StableHash` impl.)
 - #154752 (Add comment to borrow-checker)
@matthiaskrgr
Copy link
Copy Markdown
Member

@bors r-
#154757 (comment)

@rust-bors rust-bors bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 3, 2026
@malezjaa
Copy link
Copy Markdown
Contributor Author

malezjaa commented Apr 3, 2026

Should I just increase ASINH_APPROX, ACOSH_APPROX thresholds for x86 or is that not the solution?

@tgross35
Copy link
Copy Markdown
Contributor

tgross35 commented Apr 3, 2026

Ah yeah, i586 has all kinds of issues with floating point accuracy. I would do this as:

/// i586 has issues with floating point precision.
const I586: bool = cfg!(target_arch = "x86") && cfg!(not(target_feature = "sse2"));

impl ... {
    const ASINH_APPROX: Self = if i586 { /* relaxed precision */ } else { /* current precision */ };
}

Make the precision larger than what showed up in the tests to leave some flexibility in the future.

@malezjaa
Copy link
Copy Markdown
Contributor Author

malezjaa commented Apr 3, 2026

Oh also, is this only for f32 or for other types too?

@tgross35
Copy link
Copy Markdown
Contributor

tgross35 commented Apr 3, 2026

Usually that applies to f32 and f64 (not f16/f128 since they don't have the same ABI), but I guess the f64 precision is already relaxed enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

asinh and acosh return inf for very large input values

8 participants