-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Implement Integer funnel shifts #145690
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Implement Integer funnel shifts #145690
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -148,3 +148,58 @@ impl_disjoint_bitor! { | |
u8, u16, u32, u64, u128, usize, | ||
i8, i16, i32, i64, i128, isize, | ||
} | ||
|
||
#[const_trait] | ||
#[rustc_const_unstable(feature = "core_intrinsics_fallbacks", issue = "none")] | ||
pub trait FunnelShift: Copy + 'static { | ||
/// See [`super::unchecked_funnel_shl`]; we just need the trait indirection to handle | ||
/// different types since calling intrinsics with generics doesn't work. | ||
unsafe fn unchecked_funnel_shl(self, rhs: Self, shift: u32) -> Self; | ||
|
||
/// See [`super::unchecked_funnel_shr`]; we just need the trait indirection to handle | ||
/// different types since calling intrinsics with generics doesn't work. | ||
unsafe fn unchecked_funnel_shr(self, rhs: Self, shift: u32) -> Self; | ||
} | ||
|
||
macro_rules! impl_funnel_shifts { | ||
($($type:ident),*) => {$( | ||
#[rustc_const_unstable(feature = "core_intrinsics_fallbacks", issue = "none")] | ||
impl const FunnelShift for $type { | ||
#[cfg_attr(miri, track_caller)] | ||
#[inline] | ||
unsafe fn unchecked_funnel_shl(self, rhs: Self, shift: u32) -> Self { | ||
if shift == 0 { | ||
self | ||
} else { | ||
// SAFETY: our precondition is that `shift < T::BITS`, so these are valid | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also following up on @RalfJung's comment: this is the comment where you should clarify that the preconditions for the method are being tested. Assuming that That said, it might be a bit clearer to just add an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also worth commenting that while "so these are valid" is okay, you should probably be a bit more specific:
I do think that at least the last bit should be mentioned, since while we don't implement for signed integers for the moment, we do implement rotations for signed integers, and it's worth reminding people that this will not work as-is for signed integers and they have to be cast to unsigned integers first. |
||
unsafe { | ||
super::disjoint_bitor( | ||
super::unchecked_shl(self, shift), | ||
super::unchecked_shr(rhs, $type::BITS - shift), | ||
) | ||
} | ||
} | ||
} | ||
|
||
#[cfg_attr(miri, track_caller)] | ||
#[inline] | ||
unsafe fn unchecked_funnel_shr(self, rhs: Self, shift: u32) -> Self { | ||
if shift == 0 { | ||
rhs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this correct? |
||
} else { | ||
// SAFETY: our precondition is that `shift < T::BITS`, so these are valid | ||
unsafe { | ||
super::disjoint_bitor( | ||
super::unchecked_shl(self, $type::BITS - shift), | ||
super::unchecked_shr(rhs, shift), | ||
) | ||
} | ||
} | ||
} | ||
} | ||
)*}; | ||
} | ||
|
||
impl_funnel_shifts! { | ||
u8, u16, u32, u64, u128, usize | ||
} |
sayantn marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2102,6 +2102,59 @@ pub const fn saturating_add<T: Copy>(a: T, b: T) -> T; | |
#[rustc_intrinsic] | ||
pub const fn saturating_sub<T: Copy>(a: T, b: T) -> T; | ||
|
||
/// Funnel Shift left. | ||
/// | ||
/// Concatenates `a` and `b` (with `a` in the most significant half), | ||
/// creating an integer twice as wide. Then shift this integer left | ||
/// by `shift`), and extract the most significant half. If `a` and `b` | ||
/// are the same, this is equivalent to a rotate left operation. | ||
/// | ||
/// It is undefined behavior if `shift` is greater than or equal to the | ||
/// bit size of `T`. | ||
/// | ||
/// Safe versions of this intrinsic are available on the integer primitives | ||
/// via the `funnel_shl` method. For example, [`u32::funnel_shl`]. | ||
#[rustc_intrinsic] | ||
#[rustc_nounwind] | ||
#[rustc_const_unstable(feature = "funnel_shifts", issue = "145686")] | ||
folkertdev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#[unstable(feature = "funnel_shifts", issue = "145686")] | ||
#[miri::intrinsic_fallback_is_spec] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This attribute means the fallback implementation must check the size to be valid and do Please add Miri tests to ensure this works correctly: all cases of UB must be detected. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the implementation, I don't think you are checking for UB here. Seems like Please don't just blindly add attributes like this, make sure you understand them or ask what to do. Adding this attribute incorrectly introduces hard-to-find bugs into the very tools unsafe code authors trust to find bugs for them. EDIT: Or, are you relying on the checks in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also clarifying a bit: since the fallbacks work a bit differently than you'd expect, this kind of info should probably be listed in the |
||
pub const unsafe fn unchecked_funnel_shl<T: [const] fallback::FunnelShift>( | ||
a: T, | ||
b: T, | ||
shift: u32, | ||
) -> T { | ||
// SAFETY: caller ensures that `shift` is in-range | ||
unsafe { a.unchecked_funnel_shl(b, shift) } | ||
} | ||
|
||
/// Funnel Shift right. | ||
/// | ||
/// Concatenates `a` and `b` (with `a` in the most significant half), | ||
/// creating an integer twice as wide. Then shift this integer right | ||
/// by `shift` (taken modulo the bit size of `T`), and extract the | ||
/// least significant half. If `a` and `b` are the same, this is equivalent | ||
/// to a rotate right operation. | ||
/// | ||
/// It is undefined behavior if `shift` is greater than or equal to the | ||
/// bit size of `T`. | ||
/// | ||
/// Safer versions of this intrinsic are available on the integer primitives | ||
/// via the `funnel_shr` method. For example, [`u32::funnel_shr`] | ||
#[rustc_intrinsic] | ||
#[rustc_nounwind] | ||
#[rustc_const_unstable(feature = "funnel_shifts", issue = "145686")] | ||
#[unstable(feature = "funnel_shifts", issue = "145686")] | ||
#[miri::intrinsic_fallback_is_spec] | ||
pub const unsafe fn unchecked_funnel_shr<T: [const] fallback::FunnelShift>( | ||
a: T, | ||
b: T, | ||
shift: u32, | ||
) -> T { | ||
// SAFETY: caller ensures that `shift` is in-range | ||
unsafe { a.unchecked_funnel_shr(b, shift) } | ||
} | ||
|
||
/// This is an implementation detail of [`crate::ptr::read`] and should | ||
/// not be used anywhere else. See its comments for why this exists. | ||
/// | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you update the tracking issue to have signatures matching rust-lang/libs-team#642 (comment)? This should match then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done 👍🏽, could you check if it makes sense (I'm bad at writing docs etc) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's easier than that: tracking issues should show function signatures in something like an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the same implementation as
rotate_left
/rotate_right
(except with an extra argument), it might be a bit clearer to just share the same branch for both and then add a branch that chooses what arguments to use depending on which symbol is being called.This will make it a little clearer that they're both just funnel shifts, since that way if someone changes this code at all they don't have to change it in both places, since it's already deduped.