Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add f16 and f128 math functions #127027

Merged
merged 6 commits into from
Jul 31, 2024
Merged

Add f16 and f128 math functions #127027

merged 6 commits into from
Jul 31, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jun 27, 2024 •
edited
Loading

This adds intrinsics and math functions for f16 and f128 floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

try-job: aarch64-gnu

r? ghost
@rustbot label +F-f16_and_f128

@rustbot rustbot added O-unix Operating system: Unix-like 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. F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` labels Jun 27, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tgross35
Copy link
Contributor Author

@rustbot label +rla-silenced

@rustbot rustbot added the rla-silenced Silences rust-log-analyzer postings to the PR it's added on. label Jun 27, 2024
@tgross35
Copy link
Contributor Author

Still getting a failure on f16 locally (aarch64) that I can't explain, 1.0f16.powi(1) is returning a wrong result. This happens in the unit tests but doesn't seem to replicate when I copy them to a separate file, making this difficult to debug.

May as well see if this shows up in CI.

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2024
@bors
Auto merge of rust-lang#127027 - tgross35:f16-f128-math, r=<try>
4ad38c5 
Add `f16` and `f128` math functions

WIP

This will be pretty finicky among selection failures, the lowering bug, and lack of symbols. But might as well start somewhere.

Effectively blocked on const casting.

try-job: aarch64-gnu
@bors
Copy link
Contributor

bors commented Jul 12, 2024

⌛ Trying commit 0bca7c3 with merge 4ad38c5...

@tgross35
Copy link
Contributor Author

Figured out how to reproduce, the incorrect value is only returned if optimizations are on 💀

@bors cancel

@tgross35

This comment was marked as outdated.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
@bors bors 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 Jul 12, 2024
@tgross35
Copy link
Contributor Author

f16 issue on aarch: llvm/llvm-project#98665

@tgross35

This comment was marked as outdated.

Sign in to view
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 12, 2024
@bors
Auto merge of rust-lang#127027 - tgross35:f16-f128-math, r=<try>
977d16e 
Add `f16` and `f128` math functions

WIP

This will be pretty finicky among selection failures, the lowering bug, and lack of symbols. But might as well start somewhere.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
@bors

This comment was marked as outdated.

Sign in to view
@tgross35
Copy link
Contributor Author

There are probably some rough edges that I need to smooth out and some tests that need tweaking, but this PR is somewhat large so I think somebody can start taking a look at it. f16 pass on my aarch machine, I can't test f128 locally so I'm going via try jobs.

Note that this is still going to fail the check CI job until nightly clippy becomes beta next week.

r? @dtolnay

Since I know you have reviewed the math functions before. Feel free to reroll if you prefer.

@tgross35 tgross35 marked this pull request as ready for review July 12, 2024 17:49
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
@tgross35

This comment was marked as outdated.

Sign in to view
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2024
@bors
Auto merge of rust-lang#127027 - tgross35:f16-f128-math, r=<try>
eea39ef 
Add `f16` and `f128` math functions

This adds intrinsics and math functions for `f16` and `f128` floating point types. Support is quite limited and some things are broken so tests don't run on many platforms, but this provides a starting point.

Checks will fail until rust-lang#126636 makes it to beta, which should be next week (July 19).

try-job: aarch64-gnu
@bors

This comment was marked as outdated.

Sign in to view
@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 17, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Jul 17, 2024 •
edited
Loading

Thank you for the review! Yes, the Clippy ICE fix should be be in beta Friday/Saturday so that should (hopefully) fix CI.

edit: update, the bootstrap bump has unfortunately just taken a while #128083.

This was referenced Jul 18, 2024
Open
Open
@rdrpenguin04 rdrpenguin04 mentioned this pull request Jul 19, 2024
Open
@bors
Copy link
Contributor

bors commented Jul 29, 2024

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35
Specify the integer type of the powi LLVM intrinsic
c586eb6 
Since LLVM <https://reviews.llvm.org/D99439> (4c7f820, "Update
@llvm.powi to handle different int sizes for the exponent"), the size of
the integer can be specified for the `powi` intrinsic. Make use of this
so it is more obvious that integer size is consistent across all float
types.

This feature is available since LLVM 13 (October 2021). Based on
bootstrap we currently support >= 17.0, so there should be no support
problems.
@tgross35
Add math intrinsics for f16 and f128
b6d0517 
These already exist in the compiler. Expose them in core so we can add
their library functions.
@tgross35 tgross35 force-pushed the f16-f128-math branch 2 times, most recently from e06ac42 to 84c6478 Compare July 30, 2024 21:36
@tgross35
Add math functions for f16 and f128
90e63dd 
This adds missing functions for math operations on the new float types.

Platform support is pretty spotty at this point, since even platforms
with generally good support can be missing math functions.
`std/build.rs` is updated to reflect this.
@tgross35
Add core functions for f16 and f128 that require math routines
a4657dd 
`min`, `max`, and similar functions require external math routines. Add
these under the same gates as `std` math functions (`reliable_f16_math`
and `reliable_f128_math`).
@tgross35
Update comments for {f16, f32, f64, f128}::midpoint
e1d23c9 
Clarify what makes some operations not safe, and correct comment in the
default branch ("not safe" -> "safe").
@tgross35
Add a disclaimer about x86 f128 math functions
d64bbb1 
Due to a LLVM bug, `f128` math functions link successfully but LLVM
chooses the wrong symbols (`long double` symbols rather than those for
binary128).

Since this is a notable problem that may surprise a number of users, add
a note about it.

Link: llvm/llvm-project#44744
@tgross35
Copy link
Contributor Author

The stage0 bump took longer than expected but finally merged, so I was able to rebase. Since the last review I removed all the cfg(not(bootstrap)) and added a note in the docs about the x86 math bug, so users are aware of it.

One more try job to make sure nothing got broken:

@bors try

@bors
Copy link
Contributor

bors commented Jul 30, 2024

⌛ Trying commit d64bbb1 with merge 2318850...

@tgross35
Copy link
Contributor Author

x86 CI passed and it looks like the aarch64 try job has tested the libraries already, so this should be about ready. Changes were trivial since the last look so

@bors r=dtolnay

@bors
Copy link
Contributor

bors commented Jul 30, 2024

📌 Commit d64bbb1 has been approved by dtolnay

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 30, 2024
@bors
Copy link
Contributor

bors commented Jul 31, 2024

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 2318850 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 31, 2024
@bors bors merged commit 2318850 into rust-lang:master Jul 31, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 31, 2024
@tgross35
Copy link
Contributor Author

tgross35 commented Jul 31, 2024 •
edited
Loading

What the heck

https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/bors.20merged.20PR.20without.20running.20all.20tests

@tgross35
Copy link
Contributor Author

Mark reset master. I'll open a new PR.

@tgross35 tgross35 mentioned this pull request Jul 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like rla-silenced Silences rust-log-analyzer postings to the PR it's added on. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.82.0
Development

Successfully merging this pull request may close these issues.
5 participants
@tgross35 @rust-log-analyzer @bors @dtolnay @rustbot