Merge fabsf16/32/64/128 into fabs::<F>#153834
Conversation
|
Some changes occurred to the CTFE machinery Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter |
|
r? @TaKO8Ki rustbot has assigned @TaKO8Ki. Use Why was this reviewer chosen?The reviewer was selected based on:
|
fabsfN into fabs::<F>fabsf16/32/64/128 into fabs::<F>
|
r? tgross35 |
This comment has been minimized.
This comment has been minimized.
|
ah oops i was wondering why gcc wasn't working locally, guess it never installed :') ill fix this soon |
library/core/src/intrinsics/mod.rs
Outdated
| #[rustc_nounwind] | ||
| #[rustc_intrinsic] | ||
| pub const fn fabsf128(x: f128) -> f128; | ||
| pub const fn fabs<T: Copy>(x: T) -> T; |
There was a problem hiding this comment.
suggestion: like there's
rust/library/core/src/intrinsics/bounds.rs
Lines 5 to 25 in f1ceedf
| pub const fn fabs<T: Copy>(x: T) -> T; | |
| pub const fn fabs<T: bounds::FloatPrimitive>(x: T) -> T; |
or similar.
There was a problem hiding this comment.
Fixed in 74114c9; also added it to the other generic float intrinsics
There was a problem hiding this comment.
note this changes the diagnostic when these intrinsics are misused: it now gives a type error (see 043f9ad) rather than a monomorphization error; i think this is a good thing :)
| ty::Float(FloatTy::F32) | ty::Float(FloatTy::F64) => fx.bcx.ins().fabs(x), | ||
| // FIXME(bytecodealliance/wasmtime#8312): Use `fabsf16` once Cranelift | ||
| // backend lowerings are implemented. | ||
| ty::Float(FloatTy::F16) => codegen_f16_f128::abs_f16(fx, x), |
There was a problem hiding this comment.
pondering: these make me wonder if it'd be worth just having fallback mir for this?
See how there's
pub const unsafe fn disjoint_bitor<T: [const] fallback::DisjointBitOr>(a: T, b: T) -> T {for example to give that one a fallback; might be reasonable to do the same for fabs (since it's easy to implement) and backends can just fallback to that if they haven't implemented it natively for f16/f128 yet.
There was a problem hiding this comment.
(That said, it's a copysign+fabs thing only, really, since fallback MIR for most float ops is probably not a good idea. fpow is hard.)
There was a problem hiding this comment.
I'd like to have fallbacks for all of these via libcalls; have some of that in progress at #150946.
abs and copysign probably shouldn't use the libcalls though, they can copy the implementation from https://github.com/rust-lang/compiler-builtins/blob/85db9f1923007c287b894d392b880545ec70d05e/libm/src/math/generic/fabs.rs and https://github.com/rust-lang/compiler-builtins/blob/85db9f1923007c287b894d392b880545ec70d05e/libm/src/math/generic/copysign.rs.
This comment has been minimized.
This comment has been minimized.
|
btw; I saw in rust/compiler/rustc_codegen_ssa/src/errors.rs Lines 718 to 724 in 620e36a rust/compiler/rustc_codegen_ssa/src/errors.rs Lines 742 to 748 in 620e36a |
|
The diagnostic error types are quite the mess there, feel free to adjust or replace them by string-based errors if that simplifies the code. |
This comment has been minimized.
This comment has been minimized.
69be112 to
9210d78
Compare
fair enough :) removed the duplicate in ebb6dfb |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Misuse of intrinsics falls under rust-lang/compiler-team#620 -- you can feel free to rip out any inconvenient errors entirely and just have them ICE. |
| #[rustc_nounwind] | ||
| #[rustc_intrinsic] | ||
| pub const fn fabsf128(x: f128) -> f128; | ||
| pub const fn fabs<T: bounds::FloatPrimitive>(x: T) -> T; |
There was a problem hiding this comment.
@rust-lang/lang this would mark fabs as indirectly const stable for all float types, not just f32 and f64. The const implementation is uniform across all types so I have no concerns about this -- the f16 and f128 implementation is and has been ready for stable for a while, it's the non-const implementation of f16 and f128 that's not ready. ;)
Nevertheless, new intrinsics being available to stable const code requires lang approval. Not sure if you think a full FCP is required for a trivial case like this.
There was a problem hiding this comment.
btw if a FCP is required, we should throw in copysign in there, which is in the same situation: const stable for f32/f64 but not for f16/f128, and it's also a binary operation
There was a problem hiding this comment.
We should probably throw in all the intrinsics you intend to generalize that are easily implemented with apfloat. Do you have a list of them?
There was a problem hiding this comment.
Oh, I didn't realize all the f16/f128 rounding methods have already been allowed in const-stable since #143604. That may have been an accident, but it's also not really a problem.
There was a problem hiding this comment.
yep, it's fabs, copysign, minnum and maxnum; all other float intrinsics are all const stable across all float types or not const stable for any
There was a problem hiding this comment.
Personally (with my lang hat on but not speaking as team consensus) I would be fine treating this as "the previous FCP was saying that floating-point abs is allowed to be const-stable -- because it's easy to do reliably in const, being that it's only bitwise (without even NAN nondet, IIRC?) -- and how exactly that translates to specific intrinsics is a compiler/ctfe question without needing addtional lang FCPs".
But we'll see what the meeting says on Wed.
There was a problem hiding this comment.
without even NAN nondet, IIRC?
Correct, fabs just mutates the sign bit and nothing else.
|
the GCC issue here was because without type information overall the gcc handling of float intrinsics is.. not ideal right now, my goal is that as i genericize the rest of the intrinsics i can clean it up and make it simpler :) rn it's just in an in-between phase which is not super pretty |
This comment has been minimized.
This comment has been minimized.
b3e650e to
4affb1c
Compare
This comment has been minimized.
This comment has been minimized.
4affb1c to
48cec8c
Compare
This comment has been minimized.
This comment has been minimized.
48cec8c to
88a9639
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
88a9639 to
e21ccc5
Compare
|
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. |
e21ccc5 to
42c0625
Compare
There was a problem hiding this comment.
Cc @bjorn3 if you want to look at clif changes but this generally LGTM
42c0625 to
2fdefdf
Compare
There was a problem hiding this comment.
This needs the trivial lang approval for the intrinsics, and one more nit above. But aside from that, the changes here LGTM unless @RalfJung has anything else.
Add `bounds::FloatPrimitive` Exhaustive float pattern match Fix GCC use span bugs
2fdefdf to
11c673b
Compare
|
The const-eval parts LGTM. |
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
|
To be clear, the FCP covers doing this for fabs, copysign, minnum and maxnum. This PR only does the first step as that's easier for reviewing. (No other intrinsic has mixed const-stability across float types.) That said, @scottmcm suggested we could consider this covered by the previous FCP for fabs on f32 and f64. We'll see what the meeting on Wed brings. |
View all comments
Following a small conversation on Zulip (and because I'd be interested in starting to contribute on Rust), I thought I'd give a try at merging the float intrinsics :)
This PR just merges
fabsf16,fabsf32,fabsf64,fabsf128, as it felt like an easy first target.Notes:
fabsf32andfabsf64are#[rustc_intrinsic_const_stable_indirect], whilefabsf16andfabsf128aren't; becausef32/f64expect the function to be const, the generic version must be made indirectly stable too. We'd need to check with T-lang this change is ok; the only other intrinsics where there is such a mismatch isminnum,maxnumandcopysign.