Skip to content

intrinsics: unify rint, roundeven, nearbyint in a single round_ties_even intrinsic #136543

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
Merged
merged 3 commits into from
Feb 24, 2025
Merged

intrinsics: unify rint, roundeven, nearbyint in a single round_ties_even intrinsic #136543

merged 3 commits into from
Feb 24, 2025

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 4, 2025 •
edited
Loading

LLVM has three intrinsics here that all do the same thing (when used in the default FP environment). There's no reason Rust needs to copy that historically-grown mess -- let's just have one intrinsic and leave it up to the LLVM backend to decide how to lower that.

Suggested by @hanna-kruppe in #136459; Cc @tgross35

try-job: test-various

@rustbot
Copy link
Collaborator

rustbot commented Feb 4, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
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

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

rustbot commented Feb 4, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

The Miri subtree was changed

cc @rust-lang/miri

RalfJung
RalfJung commented Feb 4, 2025
View reviewed changes
compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs
sym::nearbyintf32 | sym::nearbyintf64 => fx.bcx.ins().nearest(args[0]),
sym::round_ties_even_f32 | sym::round_ties_even_f64 => {
fx.bcx.ins().nearest(args[0])
}
Copy link
Member Author

@RalfJung RalfJung Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This means the cranelift backend will actually change behavior with this PR -- the round_ties_even functions will lower to the "nearest" instruction rather than to rint libcalls. @bjorn3 is that okay?

I am also surprised to not see a case for round here (rounding half-way cases away from zero); is that the one rounding mode cranelit does not support natively?

Copy link
Contributor

@tgross35 tgross35 Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If this ever lowers to nearbyint then we do not currently provide that in libm so there may be linking problems on targets without a system libm. I don't expect this to be a problem for cranelift though.

@RalfJung RalfJung mentioned this pull request Feb 4, 2025
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ibraheemdev
Copy link
Member

r? compiler

@rustbot rustbot assigned wesleywiser and unassigned ibraheemdev Feb 11, 2025
@RalfJung
Copy link
Member Author

r? @tgross35 could you review this one?

@rustbot rustbot assigned tgross35 and unassigned wesleywiser Feb 20, 2025
tgross35
tgross35 reviewed Feb 21, 2025
View reviewed changes
Copy link
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.

r=me with an answer to https://github.com/rust-lang/rust/pull/136543/files#r1941275491 (@bjorn3).

I do still think it would be nicer to emit roundeven rather than rint for GCC (possibly also cranelift if applicable) since, given the option, a libcall that doesn't touch fpenv seems cleaner. That's blocked on a few things though, so the current version lgtm.

library/core/src/intrinsics/mod.rs Outdated
#[rustc_nounwind]
pub unsafe fn rintf16(_x: f16) -> f16 {
#[cfg(not(bootstrap))]
pub unsafe fn round_ties_even_f16(_x: f16) -> f16 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the latest on safe vs. unsafe intrinsics? There shouldn't be any preconditions here so it could probably be updated while you're at it (not necessary ofc).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I guess I can mark this safe while I am at it, though it is strangely inconsistent with the other rounding intrinsics.

@RalfJung
Copy link
Member Author

RalfJung commented Feb 22, 2025 •
edited
Loading

I do still think it would be nicer to emit roundeven rather than rint for GCC (possibly also cranelift if applicable) since, given the option, a libcall that doesn't touch fpenv seems cleaner. That's blocked on a few things though, so the current version lgtm.

This PR intentionally does not change the code we emit for f*::round_ties_even, but leaves that as a future option. :)

@RalfJung
Copy link
Member Author

@bors r=tgross35

@bors
Copy link
Collaborator

bors commented Feb 22, 2025

📌 Commit d1b34ac has been approved by tgross35

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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 22, 2025
Closed
@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Collaborator

bors commented Feb 23, 2025

⌛ Trying commit b9d0555 with merge 08959bf...

@bors
Copy link
Collaborator

bors commented Feb 23, 2025

☀️ Try build successful - checks-actions
Build commit: 08959bf (08959bfe7a4e50cc5d38c0549fd2e835b2c1831a)

@RalfJung
Copy link
Member Author

@bors r=tgross35

@bors
Copy link
Collaborator

bors commented Feb 23, 2025

📌 Commit b9d0555 has been approved by tgross35

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-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 23, 2025
@tgross35 tgross35 mentioned this pull request Feb 23, 2025
Merged
@bors bors merged commit a2bb4d7 into rust-lang:master Feb 24, 2025
7 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 24, 2025
@RalfJung RalfJung deleted the round-ties-even branch February 24, 2025 07:51
@carolynzech carolynzech mentioned this pull request Feb 27, 2025
Merged
@RalfJung RalfJung mentioned this pull request Mar 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 left review comments

Assignees

@tgross35 tgross35

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.87.0
Development

Successfully merging this pull request may close these issues.
8 participants
@RalfJung @rustbot @rust-log-analyzer @ibraheemdev @bors @matthiaskrgr @tgross35 @wesleywiser