Add fallback for intrinsics::fabs#159605
Conversation
|
Any special-casing of Miri in the standard library requires review. cc @rust-lang/miri Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr
cc @rust-lang/miri |
rustbot
added
S-waiting-on-review
| #[miri::intrinsic_fallback_is_spec] | ||
| pub const fn fabs<T: bounds::FloatPrimitive>(x: T) -> T { | ||
| // SAFETY: the implementation of `FloatPrimitive` guarantees that `T` is a | ||
| // floating-point type, and that `T::Unsigned` is the corresponding unsigned | ||
| // integer type with the same size as `T`. Both are valid for all bit patterns. | ||
| let bits: T::Unsigned = unsafe { transmute_unchecked(x) }; | ||
| let abs = bits & !T::SIGN_MASK; | ||
| // SAFETY: same as above | ||
| unsafe { transmute_unchecked(abs) } | ||
| } |
There was a problem hiding this comment.
@RalfJung does this spark joy? I'd much rather have the one-liner duplicated 4 times than the (trivial, but still) unsafety and const trait stuff that is required.
There was a problem hiding this comment.
The question is whether it sparks more or less joy than copy-paste. The status quo is definitely not joyful. ;)
But yeah I agree needing unsafe is pretty bad.
There was a problem hiding this comment.
does it spark joy now ? ^-^
pub const fn fabs<T: const bounds::FloatPrimitive>(x: T) -> T {
T::from_bits(x.to_bits() & !T::SIGN_MASK)
}There was a problem hiding this comment.
Yes that seems exactly as readable as the per-type version, just less duplicated :)
| #[rustc_const_unstable(feature = "core_intrinsics", issue = "none")] | ||
| const unsafe impl FloatPrimitive for f16 { | ||
| type Unsigned = u16; | ||
| const SIGN_MASK: Self::Unsigned = f16::SIGN_MASK; | ||
| fn to_bits(self) -> Self::Unsigned { | ||
| f16::to_bits(self) | ||
| } | ||
| fn from_bits(bits: Self::Unsigned) -> Self { | ||
| f16::from_bits(bits) | ||
| } | ||
| } |
There was a problem hiding this comment.
this can be macro'd if you'd rather i have no strong opinions on that
There was a problem hiding this comment.
Let's hold off on using macros, this seems manageable now and a macro won't work for fallbacks that need a per-type implementation (e.g. sin or fma)
| #[rustc_const_unstable(feature = "core_intrinsics", issue = "none")] | ||
| const unsafe impl FloatPrimitive for f16 { | ||
| type Unsigned = u16; | ||
| const SIGN_MASK: Self::Unsigned = f16::SIGN_MASK; | ||
| fn to_bits(self) -> Self::Unsigned { | ||
| f16::to_bits(self) | ||
| } | ||
| fn from_bits(bits: Self::Unsigned) -> Self { | ||
| f16::from_bits(bits) | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's hold off on using macros, this seems manageable now and a macro won't work for fallbacks that need a per-type implementation (e.g. sin or fma)
Either that, more a per-operation trait defined locally by a macro. |
edit: Ralf beat me to it
for those i think what's easiest (which @RalfJung ) commented on Zulip. i dont think i would format it like that, but something along the lines of intrinsic_dispatch_on_type! {
pub fn fma<T: bounds::FloatPrimitive>(x: T, y: T, z: T) -> T;
f16 => {
fma(x as f64, y as f64, z as f64) as f16
}
f32 => {
libm::fmaf(x, y, z)
}
// etc
}which then would create a trait for that specific intrinsic that does the thing, adds it to the trait bound and what not? or add it to the yes it duplicates the impl but it still allows a pretty good cleanup in the rest of the compiler which personally i think is worth the code here (also avoids duplicating comments which can be quite long and technical:) ) |
4 participants
Add a fallback for the
fabsintrinsic. Because it uses const traits and const ops it required addingrustc_const_unstable-- i'm not certain it's right but it seems to make sense. I also added#[miri::intrinsic_fallback_is_spec], though im not sure if I need to modify the const eval code to removefabshandling or to remove it from any of the backendsr? folkertdev