From 44eec1d9b0eaf7375994fff39b1150539f215f1a Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sat, 25 Feb 2023 22:47:20 -0800 Subject: [PATCH] Merge two different equality specialization traits in `core` --- library/core/src/array/equality.rs | 73 +++---------------------- library/core/src/cmp.rs | 3 ++ library/core/src/cmp/bytewise.rs | 83 +++++++++++++++++++++++++++++ library/core/src/intrinsics.rs | 4 ++ library/core/src/slice/cmp.rs | 27 +--------- tests/codegen/array-equality.rs | 30 ++++++++++- tests/codegen/slice-ref-equality.rs | 56 ++++++++++++++++++- 7 files changed, 182 insertions(+), 94 deletions(-) create mode 100644 library/core/src/cmp/bytewise.rs diff --git a/library/core/src/array/equality.rs b/library/core/src/array/equality.rs index b2c895f882c6a..d749865f76f51 100644 --- a/library/core/src/array/equality.rs +++ b/library/core/src/array/equality.rs @@ -1,6 +1,5 @@ +use crate::cmp::BytewiseEq; use crate::convert::TryInto; -use crate::num::{NonZeroI128, NonZeroI16, NonZeroI32, NonZeroI64, NonZeroI8, NonZeroIsize}; -use crate::num::{NonZeroU128, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize}; #[stable(feature = "rust1", since = "1.0.0")] impl PartialEq<[B; N]> for [A; N] @@ -144,74 +143,14 @@ impl, Other, const N: usize> SpecArrayEq for T { } } -impl, U, const N: usize> SpecArrayEq for T { +impl, U, const N: usize> SpecArrayEq for T { fn spec_eq(a: &[T; N], b: &[U; N]) -> bool { - // SAFETY: This is why `IsRawEqComparable` is an `unsafe trait`. - unsafe { - let b = &*b.as_ptr().cast::<[T; N]>(); - crate::intrinsics::raw_eq(a, b) - } + // SAFETY: Arrays are compared element-wise, and don't add any padding + // between elements, so when the elements are `BytewiseEq`, we can + // compare the entire array at once. + unsafe { crate::intrinsics::raw_eq(a, crate::mem::transmute(b)) } } fn spec_ne(a: &[T; N], b: &[U; N]) -> bool { !Self::spec_eq(a, b) } } - -/// `U` exists on here mostly because `min_specialization` didn't let me -/// repeat the `T` type parameter in the above specialization, so instead -/// the `T == U` constraint comes from the impls on this. -/// # Safety -/// - Neither `Self` nor `U` has any padding. -/// - `Self` and `U` have the same layout. -/// - `Self: PartialEq` is byte-wise (this means no floats, among other things) -#[rustc_specialization_trait] -unsafe trait IsRawEqComparable: PartialEq {} - -macro_rules! is_raw_eq_comparable { - ($($t:ty),+ $(,)?) => {$( - unsafe impl IsRawEqComparable<$t> for $t {} - )+}; -} - -// SAFETY: All the ordinary integer types have no padding, and are not pointers. -is_raw_eq_comparable!(u8, u16, u32, u64, u128, usize, i8, i16, i32, i64, i128, isize); - -// SAFETY: bool and char have *niches*, but no *padding* (and these are not pointer types), so this -// is sound -is_raw_eq_comparable!(bool, char); - -// SAFETY: Similarly, the non-zero types have a niche, but no undef and no pointers, -// and they compare like their underlying numeric type. -is_raw_eq_comparable!( - NonZeroU8, - NonZeroU16, - NonZeroU32, - NonZeroU64, - NonZeroU128, - NonZeroUsize, - NonZeroI8, - NonZeroI16, - NonZeroI32, - NonZeroI64, - NonZeroI128, - NonZeroIsize, -); - -// SAFETY: The NonZero types have the "null" optimization guaranteed, and thus -// are also safe to equality-compare bitwise inside an `Option`. -// The way `PartialOrd` is defined for `Option` means that this wouldn't work -// for `<` or `>` on the signed types, but since we only do `==` it's fine. -is_raw_eq_comparable!( - Option, - Option, - Option, - Option, - Option, - Option, - Option, - Option, - Option, - Option, - Option, - Option, -); diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index b4d58376aea5d..5b5f55d0e65b0 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -22,6 +22,9 @@ #![stable(feature = "rust1", since = "1.0.0")] +mod bytewise; +pub(crate) use bytewise::BytewiseEq; + use crate::marker::Destruct; use self::Ordering::*; diff --git a/library/core/src/cmp/bytewise.rs b/library/core/src/cmp/bytewise.rs new file mode 100644 index 0000000000000..2548d9e24c9db --- /dev/null +++ b/library/core/src/cmp/bytewise.rs @@ -0,0 +1,83 @@ +use crate::num::{NonZeroI128, NonZeroI16, NonZeroI32, NonZeroI64, NonZeroI8, NonZeroIsize}; +use crate::num::{NonZeroU128, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize}; + +/// Types where `==` & `!=` are equivalent to comparing their underlying bytes. +/// +/// Importantly, this means no floating-point types, as those have different +/// byte representations (like `-0` and `+0`) which compare as the same. +/// Since byte arrays are `Eq`, that implies that these types are probably also +/// `Eq`, but that's not technically required to use this trait. +/// +/// `Rhs` is *de facto* always `Self`, but the separate parameter is important +/// to avoid the `specializing impl repeats parameter` error when consuming this. +/// +/// # Safety +/// +/// - `Self` and `Rhs` have no padding. +/// - `Self` and `Rhs` have the same layout (size and alignment). +/// - Neither `Self` nor `Rhs` have provenance, so integer comparisons are correct. +/// - `>::{eq,ne}` are equivalent to comparing the bytes. +#[rustc_specialization_trait] +pub(crate) unsafe trait BytewiseEq: PartialEq + Sized {} + +macro_rules! is_bytewise_comparable { + ($($t:ty),+ $(,)?) => {$( + unsafe impl BytewiseEq for $t {} + )+}; +} + +// SAFETY: All the ordinary integer types have no padding, and are not pointers. +is_bytewise_comparable!(u8, u16, u32, u64, u128, usize, i8, i16, i32, i64, i128, isize); + +// SAFETY: These have *niches*, but no *padding* and no *provenance*, +// so we can compare them directly. +is_bytewise_comparable!(bool, char, super::Ordering); + +// SAFETY: Similarly, the non-zero types have a niche, but no undef and no pointers, +// and they compare like their underlying numeric type. +is_bytewise_comparable!( + NonZeroU8, + NonZeroU16, + NonZeroU32, + NonZeroU64, + NonZeroU128, + NonZeroUsize, + NonZeroI8, + NonZeroI16, + NonZeroI32, + NonZeroI64, + NonZeroI128, + NonZeroIsize, +); + +// SAFETY: The NonZero types have the "null" optimization guaranteed, and thus +// are also safe to equality-compare bitwise inside an `Option`. +// The way `PartialOrd` is defined for `Option` means that this wouldn't work +// for `<` or `>` on the signed types, but since we only do `==` it's fine. +is_bytewise_comparable!( + Option, + Option, + Option, + Option, + Option, + Option, + Option, + Option, + Option, + Option, + Option, + Option, +); + +macro_rules! is_bytewise_comparable_array_length { + ($($n:literal),+ $(,)?) => {$( + // SAFETY: Arrays have no padding between elements, so if the elements are + // `BytewiseEq`, then the whole array can be too. + unsafe impl, U> BytewiseEq<[U; $n]> for [T; $n] {} + )+}; +} + +// Frustratingly, this can't be made const-generic as it gets +// error: specializing impl repeats parameter `N` +// so just do it for a couple of plausibly-common ones. +is_bytewise_comparable_array_length!(0, 1, 2, 3, 4, 6, 8, 12, 16, 24, 32, 48, 64); diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index b1ed3b31e430c..18a90599c4db9 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -2093,6 +2093,10 @@ extern "rust-intrinsic" { /// Above some backend-decided threshold this will emit calls to `memcmp`, /// like slice equality does, instead of causing massive code size. /// + /// Since this works by comparing the underlying bytes, the actual `T` is + /// not particularly important. It will be used for its size and alignment, + /// but any validity restrictions will be ignored, not enforced. + /// /// # Safety /// /// It's UB to call this if any of the *bytes* in `*a` or `*b` are uninitialized or carry a diff --git a/library/core/src/slice/cmp.rs b/library/core/src/slice/cmp.rs index 5e1b218e507bd..7601dd3c75608 100644 --- a/library/core/src/slice/cmp.rs +++ b/library/core/src/slice/cmp.rs @@ -1,6 +1,6 @@ //! Comparison traits for `[T]`. -use crate::cmp::{self, Ordering}; +use crate::cmp::{self, BytewiseEq, Ordering}; use crate::ffi; use crate::mem; @@ -77,7 +77,7 @@ where // Use memcmp for bytewise equality when the types allow impl SlicePartialEq for [A] where - A: BytewiseEquality, + A: BytewiseEq, { fn equal(&self, other: &[B]) -> bool { if self.len() != other.len() { @@ -203,29 +203,6 @@ impl SliceOrd for u8 { } } -// Hack to allow specializing on `Eq` even though `Eq` has a method. -#[rustc_unsafe_specialization_marker] -trait MarkerEq: PartialEq {} - -impl MarkerEq for T {} - -#[doc(hidden)] -/// Trait implemented for types that can be compared for equality using -/// their bytewise representation -#[rustc_specialization_trait] -trait BytewiseEquality: MarkerEq + Copy {} - -macro_rules! impl_marker_for { - ($traitname:ident, $($ty:ty)*) => { - $( - impl $traitname<$ty> for $ty { } - )* - } -} - -impl_marker_for!(BytewiseEquality, - u8 i8 u16 i16 u32 i32 u64 i64 u128 i128 usize isize char bool); - pub(super) trait SliceContains: Sized { fn slice_contains(&self, x: &[Self]) -> bool; } diff --git a/tests/codegen/array-equality.rs b/tests/codegen/array-equality.rs index cd5e82a9205c1..abfe295f8b677 100644 --- a/tests/codegen/array-equality.rs +++ b/tests/codegen/array-equality.rs @@ -1,4 +1,4 @@ -// compile-flags: -O +// compile-flags: -O -Z merge-functions=disabled // only-x86_64 #![crate_type = "lib"] @@ -43,6 +43,15 @@ pub fn array_eq_long(a: &[u16; 1234], b: &[u16; 1234]) -> bool { a == b } +// CHECK-LABEL: @array_char_eq +#[no_mangle] +pub fn array_char_eq(a: [char; 2], b: [char; 2]) -> bool { + // CHECK-NEXT: start: + // CHECK-NEXT: %[[EQ:.+]] = icmp eq i64 %0, %1 + // CHECK-NEXT: ret i1 %[[EQ]] + a == b +} + // CHECK-LABEL: @array_eq_zero_short(i48 #[no_mangle] pub fn array_eq_zero_short(x: [u16; 3]) -> bool { @@ -52,6 +61,25 @@ pub fn array_eq_zero_short(x: [u16; 3]) -> bool { x == [0; 3] } +// CHECK-LABEL: @array_eq_none_short(i40 +#[no_mangle] +pub fn array_eq_none_short(x: [Option; 5]) -> bool { + // CHECK-NEXT: start: + // CHECK-NEXT: %[[EQ:.+]] = icmp eq i40 %0, 0 + // CHECK-NEXT: ret i1 %[[EQ]] + x == [None; 5] +} + +// CHECK-LABEL: @array_eq_zero_nested( +#[no_mangle] +pub fn array_eq_zero_nested(x: [[u8; 3]; 3]) -> bool { + // CHECK: %[[VAL:.+]] = load i72 + // CHECK-SAME: align 1 + // CHECK: %[[EQ:.+]] = icmp eq i72 %[[VAL]], 0 + // CHECK: ret i1 %[[EQ]] + x == [[0; 3]; 3] +} + // CHECK-LABEL: @array_eq_zero_mid( #[no_mangle] pub fn array_eq_zero_mid(x: [u16; 8]) -> bool { diff --git a/tests/codegen/slice-ref-equality.rs b/tests/codegen/slice-ref-equality.rs index 47fde12bf3036..8f0adab35e710 100644 --- a/tests/codegen/slice-ref-equality.rs +++ b/tests/codegen/slice-ref-equality.rs @@ -1,7 +1,10 @@ -// compile-flags: -C opt-level=3 -Zmerge-functions=disabled +// compile-flags: -O -Zmerge-functions=disabled +// ignore-debug (the extra assertions get in the way) #![crate_type = "lib"] +use std::num::{NonZeroI16, NonZeroU32}; + // #71602 reported a simple array comparison just generating a loop. // This was originally fixed by ensuring it generates a single bcmp, // but we now generate it as a load+icmp instead. `is_zero_slice` was @@ -36,3 +39,54 @@ pub fn is_zero_array(data: &[u8; 4]) -> bool { // CHECK-NEXT: ret i1 %[[EQ]] *data == [0; 4] } + +// The following test the extra specializations to make sure that slice +// equality for non-byte types also just emit a `bcmp`, not a loop. + +// CHECK-LABEL: @eq_slice_of_nested_u8( +// CHECK-SAME: [[USIZE:i16|i32|i64]] noundef %1 +// CHECK-SAME: [[USIZE]] noundef %3 +#[no_mangle] +fn eq_slice_of_nested_u8(x: &[[u8; 3]], y: &[[u8; 3]]) -> bool { + // CHECK: icmp eq [[USIZE]] %1, %3 + // CHECK: %[[BYTES:.+]] = mul nsw [[USIZE]] %1, 3 + // CHECK: tail call{{( noundef)?}} i32 @{{bcmp|memcmp}}({{i8\*|ptr}} + // CHECK-SAME: , [[USIZE]]{{( noundef)?}} %[[BYTES]]) + x == y +} + +// CHECK-LABEL: @eq_slice_of_i32( +// CHECK-SAME: [[USIZE:i16|i32|i64]] noundef %1 +// CHECK-SAME: [[USIZE]] noundef %3 +#[no_mangle] +fn eq_slice_of_i32(x: &[i32], y: &[i32]) -> bool { + // CHECK: icmp eq [[USIZE]] %1, %3 + // CHECK: %[[BYTES:.+]] = shl nsw [[USIZE]] %1, 2 + // CHECK: tail call{{( noundef)?}} i32 @{{bcmp|memcmp}}({{i32\*|ptr}} + // CHECK-SAME: , [[USIZE]]{{( noundef)?}} %[[BYTES]]) + x == y +} + +// CHECK-LABEL: @eq_slice_of_nonzero( +// CHECK-SAME: [[USIZE:i16|i32|i64]] noundef %1 +// CHECK-SAME: [[USIZE]] noundef %3 +#[no_mangle] +fn eq_slice_of_nonzero(x: &[NonZeroU32], y: &[NonZeroU32]) -> bool { + // CHECK: icmp eq [[USIZE]] %1, %3 + // CHECK: %[[BYTES:.+]] = shl nsw [[USIZE]] %1, 2 + // CHECK: tail call{{( noundef)?}} i32 @{{bcmp|memcmp}}({{i32\*|ptr}} + // CHECK-SAME: , [[USIZE]]{{( noundef)?}} %[[BYTES]]) + x == y +} + +// CHECK-LABEL: @eq_slice_of_option_of_nonzero( +// CHECK-SAME: [[USIZE:i16|i32|i64]] noundef %1 +// CHECK-SAME: [[USIZE]] noundef %3 +#[no_mangle] +fn eq_slice_of_option_of_nonzero(x: &[Option], y: &[Option]) -> bool { + // CHECK: icmp eq [[USIZE]] %1, %3 + // CHECK: %[[BYTES:.+]] = shl nsw [[USIZE]] %1, 1 + // CHECK: tail call{{( noundef)?}} i32 @{{bcmp|memcmp}}({{i16\*|ptr}} + // CHECK-SAME: , [[USIZE]]{{( noundef)?}} %[[BYTES]]) + x == y +}