Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Addf16 andf128 math functions#127027

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bors merged 6 commits intorust-lang:masterfromtgross35:f16-f128-math
Jul 31, 2024
Merged

Conversation

@tgross35
Copy link
Contributor

@tgross35tgross35 commentedJun 27, 2024•
edited
Loading

This adds intrinsics and math functions forf16 andf128 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

@rustbotrustbot added O-unixOperating system: Unix-like S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. T-libsRelevant to the library team, which will review and decide on the PR/issue. F-f16_and_f128`#![feature(f16)]`, `#![feature(f128)]` labelsJun 27, 2024
@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
ContributorAuthor

@rustbot label +rla-silenced

@rustbotrustbot added the rla-silencedSilences rust-log-analyzer postings to the PR it's added on. labelJun 27, 2024
@tgross35
Copy link
ContributorAuthor

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 requestJul 12, 2024
Add `f16` and `f128` math functionsWIPThis 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
Collaborator

⌛ Trying commit0bca7c3 with merge4ad38c5...

@tgross35
Copy link
ContributorAuthor

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

@bors cancel

@tgross35

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@borsbors added S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties. labelsJul 12, 2024
@tgross35
Copy link
ContributorAuthor

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

@tgross35

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull requestJul 12, 2024
Add `f16` and `f128` math functionsWIPThis will be pretty finicky among selection failures, the lowering bug, and lack of symbols. But might as well start somewhere.Checks will fail untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
@bors

This comment was marked as outdated.

@tgross35
Copy link
ContributorAuthor

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.

@tgross35tgross35 marked this pull request as ready for reviewJuly 12, 2024 17:49
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@tgross35

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull requestJul 16, 2024
Add `f16` and `f128` math functionsThis 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 untilrust-lang#126636 makes it to beta, which should be next week (July 19).try-job: aarch64-gnu
@bors

This comment was marked as outdated.

@rustbotrustbot added S-blockedStatus: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-authorStatus: This is awaiting some action (such as code changes or more information) from the author. labelsJul 17, 2024
@tgross35
Copy link
ContributorAuthor

tgross35 commentedJul 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.

@bors
Copy link
Collaborator

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

Since LLVM <https://reviews.llvm.org/D99439> (4c7f820, "Update@llvm.powi to handle different int sizes for the exponent"), the size ofthe integer can be specified for the `powi` intrinsic. Make use of thisso it is more obvious that integer size is consistent across all floattypes.This feature is available since LLVM 13 (October 2021). Based onbootstrap we currently support >= 17.0, so there should be no supportproblems.
These already exist in the compiler. Expose them in core so we can addtheir library functions.
@tgross35tgross35force-pushed thef16-f128-math branch 2 times, most recently frome06ac42 to84c6478CompareJuly 30, 2024 21:36
This adds missing functions for math operations on the new float types.Platform support is pretty spotty at this point, since even platformswith generally good support can be missing math functions.`std/build.rs` is updated to reflect this.
`min`, `max`, and similar functions require external math routines. Addthese under the same gates as `std` math functions (`reliable_f16_math`and `reliable_f128_math`).
Clarify what makes some operations not safe, and correct comment in thedefault branch ("not safe" -> "safe").
Due to a LLVM bug, `f128` math functions link successfully but LLVMchooses the wrong symbols (`long double` symbols rather than those forbinary128).Since this is a notable problem that may surprise a number of users, adda note about it.Link:llvm/llvm-project#44744
@tgross35
Copy link
ContributorAuthor

The stage0 bump took longer than expected but finally merged, so I was able to rebase. Since the last review I removed all thecfg(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
Collaborator

⌛ Trying commitd64bbb1 with merge2318850...

@tgross35
Copy link
ContributorAuthor

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
Collaborator

📌 Commitd64bbb1 has been approved bydtolnay

It is now in thequeue for this repository.

@borsbors added S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blockedStatus: Blocked on something else such as an RFC or other implementation work. labelsJul 30, 2024
@bors
Copy link
Collaborator

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

@borsbors added the merged-by-borsThis PR was explicitly merged by bors. labelJul 31, 2024
@borsbors merged commit2318850 intorust-lang:masterJul 31, 2024
@rustbotrustbot added this to the1.82.0 milestoneJul 31, 2024
@tgross35
Copy link
ContributorAuthor

tgross35 commentedJul 31, 2024•
edited
Loading

@tgross35
Copy link
ContributorAuthor

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

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

Reviewers

@dtolnaydtolnaydtolnay approved these changes

Assignees

@dtolnaydtolnay

Labels

F-f16_and_f128`#![feature(f16)]`, `#![feature(f128)]`merged-by-borsThis PR was explicitly merged by bors.O-unixOperating system: Unix-likerla-silencedSilences rust-log-analyzer postings to the PR it's added on.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-libsRelevant 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
[8]ページ先頭
©2009-2025 Movatter.jp