intrinsics: unify rint, roundeven, nearbyint in a single round_ties_even intrinsic #136543
Conversation
|
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @rust-lang/wg-const-eval The Miri subtree was changed cc @rust-lang/miri |
RalfJung
force-pushed
the
round-ties-even
branch
from
7d67210 to
6015136
Compare
| 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]) | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
force-pushed
the
round-ties-even
branch
from
6015136 to
6f39172
Compare
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
round-ties-even
branch
from
6f39172 to
7d04265
Compare
This comment has been minimized.
This comment has been minimized.
RalfJung
force-pushed
the
round-ties-even
branch
from
7d04265 to
04e7a10
Compare
|
r? compiler |
|
r? @tgross35 could you review this one? |
tgross35
left a comment
tgross35
left a comment
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Sure I guess I can mark this safe while I am at it, though it is strangely inconsistent with the other rounding intrinsics.
This PR intentionally does not change the code we emit for |
|
@bors r=tgross35 |
bors
added
S-waiting-on-bors
RalfJung
force-pushed
the
round-ties-even
branch
from
351c587 to
b9d0555
Compare
|
@bors try |
|
☀️ Try build successful - checks-actions |
|
@bors r=tgross35 |
bors
added
S-waiting-on-bors
8 participants
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