From d05eafae2fcc05bd64ab094a1352a5c16df3106e Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 30 May 2021 10:23:50 -0700 Subject: [PATCH 1/9] Move the `PartialEq` and `Eq` impls for arrays to a separate file --- library/core/src/array/equality.rs | 111 ++++++++++++++++++++++++++++ library/core/src/array/mod.rs | 113 +---------------------------- 2 files changed, 112 insertions(+), 112 deletions(-) create mode 100644 library/core/src/array/equality.rs diff --git a/library/core/src/array/equality.rs b/library/core/src/array/equality.rs new file mode 100644 index 0000000000000..dcd78e7a245d4 --- /dev/null +++ b/library/core/src/array/equality.rs @@ -0,0 +1,111 @@ +#[stable(feature = "rust1", since = "1.0.0")] +impl PartialEq<[B; N]> for [A; N] +where + A: PartialEq, +{ + #[inline] + fn eq(&self, other: &[B; N]) -> bool { + self[..] == other[..] + } + #[inline] + fn ne(&self, other: &[B; N]) -> bool { + self[..] != other[..] + } +} + +#[stable(feature = "rust1", since = "1.0.0")] +impl PartialEq<[B]> for [A; N] +where + A: PartialEq, +{ + #[inline] + fn eq(&self, other: &[B]) -> bool { + self[..] == other[..] + } + #[inline] + fn ne(&self, other: &[B]) -> bool { + self[..] != other[..] + } +} + +#[stable(feature = "rust1", since = "1.0.0")] +impl PartialEq<[A; N]> for [B] +where + B: PartialEq, +{ + #[inline] + fn eq(&self, other: &[A; N]) -> bool { + self[..] == other[..] + } + #[inline] + fn ne(&self, other: &[A; N]) -> bool { + self[..] != other[..] + } +} + +#[stable(feature = "rust1", since = "1.0.0")] +impl PartialEq<&[B]> for [A; N] +where + A: PartialEq, +{ + #[inline] + fn eq(&self, other: &&[B]) -> bool { + self[..] == other[..] + } + #[inline] + fn ne(&self, other: &&[B]) -> bool { + self[..] != other[..] + } +} + +#[stable(feature = "rust1", since = "1.0.0")] +impl PartialEq<[A; N]> for &[B] +where + B: PartialEq, +{ + #[inline] + fn eq(&self, other: &[A; N]) -> bool { + self[..] == other[..] + } + #[inline] + fn ne(&self, other: &[A; N]) -> bool { + self[..] != other[..] + } +} + +#[stable(feature = "rust1", since = "1.0.0")] +impl PartialEq<&mut [B]> for [A; N] +where + A: PartialEq, +{ + #[inline] + fn eq(&self, other: &&mut [B]) -> bool { + self[..] == other[..] + } + #[inline] + fn ne(&self, other: &&mut [B]) -> bool { + self[..] != other[..] + } +} + +#[stable(feature = "rust1", since = "1.0.0")] +impl PartialEq<[A; N]> for &mut [B] +where + B: PartialEq, +{ + #[inline] + fn eq(&self, other: &[A; N]) -> bool { + self[..] == other[..] + } + #[inline] + fn ne(&self, other: &[A; N]) -> bool { + self[..] != other[..] + } +} + +// NOTE: some less important impls are omitted to reduce code bloat +// __impl_slice_eq2! { [A; $N], &'b [B; $N] } +// __impl_slice_eq2! { [A; $N], &'b mut [B; $N] } + +#[stable(feature = "rust1", since = "1.0.0")] +impl Eq for [T; N] {} diff --git a/library/core/src/array/mod.rs b/library/core/src/array/mod.rs index 030b42a53d05d..32d344010aafd 100644 --- a/library/core/src/array/mod.rs +++ b/library/core/src/array/mod.rs @@ -14,6 +14,7 @@ use crate::mem::{self, MaybeUninit}; use crate::ops::{Index, IndexMut}; use crate::slice::{Iter, IterMut}; +mod equality; mod iter; #[stable(feature = "array_value_iter", since = "1.51.0")] @@ -230,118 +231,6 @@ where } } -#[stable(feature = "rust1", since = "1.0.0")] -impl PartialEq<[B; N]> for [A; N] -where - A: PartialEq, -{ - #[inline] - fn eq(&self, other: &[B; N]) -> bool { - self[..] == other[..] - } - #[inline] - fn ne(&self, other: &[B; N]) -> bool { - self[..] != other[..] - } -} - -#[stable(feature = "rust1", since = "1.0.0")] -impl PartialEq<[B]> for [A; N] -where - A: PartialEq, -{ - #[inline] - fn eq(&self, other: &[B]) -> bool { - self[..] == other[..] - } - #[inline] - fn ne(&self, other: &[B]) -> bool { - self[..] != other[..] - } -} - -#[stable(feature = "rust1", since = "1.0.0")] -impl PartialEq<[A; N]> for [B] -where - B: PartialEq, -{ - #[inline] - fn eq(&self, other: &[A; N]) -> bool { - self[..] == other[..] - } - #[inline] - fn ne(&self, other: &[A; N]) -> bool { - self[..] != other[..] - } -} - -#[stable(feature = "rust1", since = "1.0.0")] -impl PartialEq<&[B]> for [A; N] -where - A: PartialEq, -{ - #[inline] - fn eq(&self, other: &&[B]) -> bool { - self[..] == other[..] - } - #[inline] - fn ne(&self, other: &&[B]) -> bool { - self[..] != other[..] - } -} - -#[stable(feature = "rust1", since = "1.0.0")] -impl PartialEq<[A; N]> for &[B] -where - B: PartialEq, -{ - #[inline] - fn eq(&self, other: &[A; N]) -> bool { - self[..] == other[..] - } - #[inline] - fn ne(&self, other: &[A; N]) -> bool { - self[..] != other[..] - } -} - -#[stable(feature = "rust1", since = "1.0.0")] -impl PartialEq<&mut [B]> for [A; N] -where - A: PartialEq, -{ - #[inline] - fn eq(&self, other: &&mut [B]) -> bool { - self[..] == other[..] - } - #[inline] - fn ne(&self, other: &&mut [B]) -> bool { - self[..] != other[..] - } -} - -#[stable(feature = "rust1", since = "1.0.0")] -impl PartialEq<[A; N]> for &mut [B] -where - B: PartialEq, -{ - #[inline] - fn eq(&self, other: &[A; N]) -> bool { - self[..] == other[..] - } - #[inline] - fn ne(&self, other: &[A; N]) -> bool { - self[..] != other[..] - } -} - -// NOTE: some less important impls are omitted to reduce code bloat -// __impl_slice_eq2! { [A; $N], &'b [B; $N] } -// __impl_slice_eq2! { [A; $N], &'b mut [B; $N] } - -#[stable(feature = "rust1", since = "1.0.0")] -impl Eq for [T; N] {} - #[stable(feature = "rust1", since = "1.0.0")] impl PartialOrd for [T; N] { #[inline] From 2456495a260827217d3c612d6c577c2f165c61eb Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 30 May 2021 10:25:41 -0700 Subject: [PATCH 2/9] Stop generating `alloca`s+`memcmp` for simple array equality --- compiler/rustc_codegen_llvm/src/context.rs | 5 ++ compiler/rustc_codegen_llvm/src/intrinsic.rs | 26 +++++++++ .../rustc_mir/src/interpret/intrinsics.rs | 18 +++++++ compiler/rustc_span/src/symbol.rs | 1 + compiler/rustc_typeck/src/check/intrinsic.rs | 8 +++ library/core/src/array/equality.rs | 53 ++++++++++++++++++- library/core/src/intrinsics.rs | 16 ++++++ src/test/codegen/array-equality.rs | 36 +++++++++++++ src/test/codegen/slice-ref-equality.rs | 19 ++++++- .../intrinsic-raw_eq-const-padding.rs | 12 +++++ .../intrinsic-raw_eq-const-padding.stderr | 21 ++++++++ .../ui/intrinsics/intrinsic-raw_eq-const.rs | 27 ++++++++++ 12 files changed, 238 insertions(+), 4 deletions(-) create mode 100644 src/test/codegen/array-equality.rs create mode 100644 src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.rs create mode 100644 src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.stderr create mode 100644 src/test/ui/intrinsics/intrinsic-raw_eq-const.rs diff --git a/compiler/rustc_codegen_llvm/src/context.rs b/compiler/rustc_codegen_llvm/src/context.rs index f662887abf820..d1aecd32e2f2d 100644 --- a/compiler/rustc_codegen_llvm/src/context.rs +++ b/compiler/rustc_codegen_llvm/src/context.rs @@ -500,6 +500,7 @@ impl CodegenCx<'b, 'tcx> { let t_i32 = self.type_i32(); let t_i64 = self.type_i64(); let t_i128 = self.type_i128(); + let t_isize = self.type_isize(); let t_f32 = self.type_f32(); let t_f64 = self.type_f64(); @@ -712,6 +713,10 @@ impl CodegenCx<'b, 'tcx> { ifn!("llvm.assume", fn(i1) -> void); ifn!("llvm.prefetch", fn(i8p, t_i32, t_i32, t_i32) -> void); + // This isn't an "LLVM intrinsic", but LLVM's optimization passes + // recognize it like one and we assume it exists in `core::slice::cmp` + ifn!("memcmp", fn(i8p, i8p, t_isize) -> t_i32); + // variadic intrinsics ifn!("llvm.va_start", fn(i8p) -> void); ifn!("llvm.va_end", fn(i8p) -> void); diff --git a/compiler/rustc_codegen_llvm/src/intrinsic.rs b/compiler/rustc_codegen_llvm/src/intrinsic.rs index 1fb201eda6bb0..615295e96e116 100644 --- a/compiler/rustc_codegen_llvm/src/intrinsic.rs +++ b/compiler/rustc_codegen_llvm/src/intrinsic.rs @@ -296,6 +296,32 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { } } + sym::raw_eq => { + let tp_ty = substs.type_at(0); + let (size, align) = self.size_and_align_of(tp_ty); + let a = args[0].immediate(); + let b = args[1].immediate(); + if size.bytes() == 0 { + self.const_bool(true) + } else if size > self.data_layout().pointer_size * 4 { + let i8p_ty = self.type_i8p(); + let a_ptr = self.bitcast(a, i8p_ty); + let b_ptr = self.bitcast(b, i8p_ty); + let n = self.const_usize(size.bytes()); + let llfn = self.get_intrinsic("memcmp"); + let cmp = self.call(llfn, &[a_ptr, b_ptr, n], None); + self.icmp(IntPredicate::IntEQ, cmp, self.const_i32(0)) + } else { + let integer_ty = self.type_ix(size.bits()); + let ptr_ty = self.type_ptr_to(integer_ty); + let a_ptr = self.bitcast(a, ptr_ty); + let a_val = self.load(a_ptr, align); + let b_ptr = self.bitcast(b, ptr_ty); + let b_val = self.load(b_ptr, align); + self.icmp(IntPredicate::IntEQ, a_val, b_val) + } + } + _ if name_str.starts_with("simd_") => { match generic_simd_intrinsic(self, name, callee_ty, args, ret_ty, llret_ty, span) { Ok(llval) => llval, diff --git a/compiler/rustc_mir/src/interpret/intrinsics.rs b/compiler/rustc_mir/src/interpret/intrinsics.rs index 4e4166dad50e2..5dd679b8912ce 100644 --- a/compiler/rustc_mir/src/interpret/intrinsics.rs +++ b/compiler/rustc_mir/src/interpret/intrinsics.rs @@ -472,6 +472,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { throw_ub_format!("`assume` intrinsic called with `false`"); } } + sym::raw_eq => { + let result = self.raw_eq_intrinsic(&args[0], &args[1])?; + self.write_scalar(result, dest)?; + } _ => return Ok(false), } @@ -559,4 +563,18 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.memory.copy(src, align, dst, align, size, nonoverlapping) } + + pub(crate) fn raw_eq_intrinsic( + &mut self, + lhs: &OpTy<'tcx, >::PointerTag>, + rhs: &OpTy<'tcx, >::PointerTag>, + ) -> InterpResult<'tcx, Scalar> { + let layout = self.layout_of(lhs.layout.ty.builtin_deref(true).unwrap().ty)?; + + let lhs = self.read_scalar(lhs)?.check_init()?; + let rhs = self.read_scalar(rhs)?.check_init()?; + let lhs_bytes = self.memory.read_bytes(lhs, layout.size)?; + let rhs_bytes = self.memory.read_bytes(rhs, layout.size)?; + Ok(Scalar::Int((lhs_bytes == rhs_bytes).into())) + } } diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index edb97d70517b0..3ab32fe418db1 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -933,6 +933,7 @@ symbols! { quote, range_inclusive_new, raw_dylib, + raw_eq, raw_identifiers, raw_ref_op, re_rebalance_coherence, diff --git a/compiler/rustc_typeck/src/check/intrinsic.rs b/compiler/rustc_typeck/src/check/intrinsic.rs index 882d5d54b7c9f..18ccaf79d32c8 100644 --- a/compiler/rustc_typeck/src/check/intrinsic.rs +++ b/compiler/rustc_typeck/src/check/intrinsic.rs @@ -380,6 +380,14 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) { sym::nontemporal_store => (1, vec![tcx.mk_mut_ptr(param(0)), param(0)], tcx.mk_unit()), + sym::raw_eq => { + let param_count = if intrinsic_name == sym::raw_eq { 2 } else { 1 }; + let br = ty::BoundRegion { var: ty::BoundVar::from_u32(0), kind: ty::BrAnon(0) }; + let param_ty = + tcx.mk_imm_ref(tcx.mk_region(ty::ReLateBound(ty::INNERMOST, br)), param(0)); + (1, vec![param_ty; param_count], tcx.types.bool) + } + other => { tcx.sess.emit_err(UnrecognizedIntrinsicFunction { span: it.span, name: other }); return; diff --git a/library/core/src/array/equality.rs b/library/core/src/array/equality.rs index dcd78e7a245d4..6d66b9e2f2780 100644 --- a/library/core/src/array/equality.rs +++ b/library/core/src/array/equality.rs @@ -5,11 +5,11 @@ where { #[inline] fn eq(&self, other: &[B; N]) -> bool { - self[..] == other[..] + SpecArrayEq::spec_eq(self, other) } #[inline] fn ne(&self, other: &[B; N]) -> bool { - self[..] != other[..] + SpecArrayEq::spec_ne(self, other) } } @@ -109,3 +109,52 @@ where #[stable(feature = "rust1", since = "1.0.0")] impl Eq for [T; N] {} + +trait SpecArrayEq: Sized { + fn spec_eq(a: &[Self; N], b: &[Other; N]) -> bool; + fn spec_ne(a: &[Self; N], b: &[Other; N]) -> bool; +} + +impl, Other, const N: usize> SpecArrayEq for T { + default fn spec_eq(a: &[Self; N], b: &[Other; N]) -> bool { + a[..] == b[..] + } + default fn spec_ne(a: &[Self; N], b: &[Other; N]) -> bool { + a[..] != b[..] + } +} + +impl + IsRawEqComparable, U, const N: usize> SpecArrayEq for T { + #[cfg(bootstrap)] + fn spec_eq(a: &[T; N], b: &[U; N]) -> bool { + a[..] == b[..] + } + #[cfg(not(bootstrap))] + 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) + } + } + 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 {} + +macro_rules! is_raw_comparable { + ($($t:ty),+) => {$( + unsafe impl IsRawEqComparable<$t> for $t {} + )+}; +} +is_raw_comparable!(bool, char, u8, u16, u32, u64, u128, usize, i8, i16, i32, i64, i128, isize); diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index c5a4bbd320804..7d2c278aa0523 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -1913,6 +1913,22 @@ extern "rust-intrinsic" { /// Allocate at compile time. Should not be called at runtime. #[rustc_const_unstable(feature = "const_heap", issue = "79597")] pub fn const_allocate(size: usize, align: usize) -> *mut u8; + + /// Determines whether the raw bytes of the two values are equal. + /// + /// The is particularly handy for arrays, since it allows things like just + /// comparing `i96`s instead of forcing `alloca`s for `[6 x i16]`. + /// + /// Above some backend-decided threshold this will emit calls to `memcmp`, + /// like slice equality does, instead of causing massive code size. + /// + /// # Safety + /// + /// This doesn't take into account padding, so if `T` has padding + /// the result will be `undef`, which cannot be exposed to safe code. + #[cfg(not(bootstrap))] + #[rustc_const_unstable(feature = "const_intrinsic_raw_eq", issue = "none")] + pub fn raw_eq(a: &T, b: &T) -> bool; } // Some functions are defined here because they accidentally got made diff --git a/src/test/codegen/array-equality.rs b/src/test/codegen/array-equality.rs new file mode 100644 index 0000000000000..6a9fb5c8f864d --- /dev/null +++ b/src/test/codegen/array-equality.rs @@ -0,0 +1,36 @@ +// compile-flags: -O +// only-x86_64 + +#![crate_type = "lib"] + +// CHECK-LABEL: @array_eq_value +#[no_mangle] +pub fn array_eq_value(a: [u16; 6], b: [u16; 6]) -> bool { + // CHECK-NEXT: start: + // CHECK-NEXT: %2 = icmp eq i96 %0, %1 + // CHECK-NEXT: ret i1 %2 + a == b +} + +// CHECK-LABEL: @array_eq_ref +#[no_mangle] +pub fn array_eq_ref(a: &[u16; 6], b: &[u16; 6]) -> bool { + // CHECK: start: + // CHECK: load i96, i96* %{{.+}}, align 2 + // CHECK: load i96, i96* %{{.+}}, align 2 + // CHECK: icmp eq i96 + // CHECK-NEXT: ret + a == b +} + +// CHECK-LABEL: @array_eq_long +#[no_mangle] +pub fn array_eq_long(a: &[u16; 1234], b: &[u16; 1234]) -> bool { + // CHECK-NEXT: start: + // CHECK-NEXT: bitcast + // CHECK-NEXT: bitcast + // CHECK-NEXT: %[[CMP:.+]] = tail call i32 @{{bcmp|memcmp}}(i8* nonnull dereferenceable(2468) %{{.+}}, i8* nonnull dereferenceable(2468) %{{.+}}, i64 2468) + // CHECK-NEXT: %[[EQ:.+]] = icmp eq i32 %[[CMP]], 0 + // CHECK-NEXT: ret i1 %[[EQ]] + a == b +} diff --git a/src/test/codegen/slice-ref-equality.rs b/src/test/codegen/slice-ref-equality.rs index acc7879e7b189..1f99ac7342b39 100644 --- a/src/test/codegen/slice-ref-equality.rs +++ b/src/test/codegen/slice-ref-equality.rs @@ -2,15 +2,30 @@ #![crate_type = "lib"] -// #71602: check that slice equality just generates a single bcmp +// #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 instead. `is_zero_slice` was +// tweaked to still test the case of comparison against a slice, +// and `is_zero_array` tests the new array-specific behaviour. // CHECK-LABEL: @is_zero_slice #[no_mangle] pub fn is_zero_slice(data: &[u8; 4]) -> bool { - // CHECK: start: + // CHECK: : // CHECK-NEXT: %{{.+}} = getelementptr {{.+}} // CHECK-NEXT: %[[BCMP:.+]] = tail call i32 @{{bcmp|memcmp}}({{.+}}) // CHECK-NEXT: %[[EQ:.+]] = icmp eq i32 %[[BCMP]], 0 // CHECK-NEXT: ret i1 %[[EQ]] + &data[..] == [0; 4] +} + +// CHECK-LABEL: @is_zero_array +#[no_mangle] +pub fn is_zero_array(data: &[u8; 4]) -> bool { + // CHECK: start: + // CHECK-NEXT: %[[PTR:.+]] = bitcast [4 x i8]* {{.+}} to i32* + // CHECK-NEXT: %[[LOAD:.+]] = load i32, i32* %[[PTR]], align 1 + // CHECK-NEXT: %[[EQ:.+]] = icmp eq i32 %[[LOAD]], 0 + // CHECK-NEXT: ret i1 %[[EQ]] *data == [0; 4] } diff --git a/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.rs b/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.rs new file mode 100644 index 0000000000000..ec1c47cfaea9f --- /dev/null +++ b/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.rs @@ -0,0 +1,12 @@ +#![feature(core_intrinsics)] +#![feature(const_intrinsic_raw_eq)] +#![deny(const_err)] + +const BAD_RAW_EQ_CALL: bool = unsafe { + std::intrinsics::raw_eq(&(1_u8, 2_u16), &(1_u8, 2_u16)) +//~^ ERROR any use of this value will cause an error +//~| WARNING this was previously accepted by the compiler but is being phased out +}; + +pub fn main() { +} diff --git a/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.stderr b/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.stderr new file mode 100644 index 0000000000000..74df99a69d1fa --- /dev/null +++ b/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.stderr @@ -0,0 +1,21 @@ +error: any use of this value will cause an error + --> $DIR/intrinsic-raw_eq-const-padding.rs:6:5 + | +LL | / const BAD_RAW_EQ_CALL: bool = unsafe { +LL | | std::intrinsics::raw_eq(&(1_u8, 2_u16), &(1_u8, 2_u16)) + | | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reading 4 bytes of memory starting at alloc2, but 1 byte is uninitialized starting at alloc2+0x1, and this operation requires initialized memory +LL | | +LL | | +LL | | }; + | |__- + | +note: the lint level is defined here + --> $DIR/intrinsic-raw_eq-const-padding.rs:3:9 + | +LL | #![deny(const_err)] + | ^^^^^^^^^ + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #71800 + +error: aborting due to previous error + diff --git a/src/test/ui/intrinsics/intrinsic-raw_eq-const.rs b/src/test/ui/intrinsics/intrinsic-raw_eq-const.rs new file mode 100644 index 0000000000000..8ea954673020e --- /dev/null +++ b/src/test/ui/intrinsics/intrinsic-raw_eq-const.rs @@ -0,0 +1,27 @@ +// run-pass + +#![feature(core_intrinsics)] +#![feature(const_intrinsic_raw_eq)] +#![deny(const_err)] + +pub fn main() { + use std::intrinsics::raw_eq; + + const RAW_EQ_I32_TRUE: bool = unsafe { raw_eq(&42_i32, &42) }; + assert!(RAW_EQ_I32_TRUE); + + const RAW_EQ_I32_FALSE: bool = unsafe { raw_eq(&4_i32, &2) }; + assert!(!RAW_EQ_I32_FALSE); + + const RAW_EQ_CHAR_TRUE: bool = unsafe { raw_eq(&'a', &'a') }; + assert!(RAW_EQ_CHAR_TRUE); + + const RAW_EQ_CHAR_FALSE: bool = unsafe { raw_eq(&'a', &'A') }; + assert!(!RAW_EQ_CHAR_FALSE); + + const RAW_EQ_ARRAY_TRUE: bool = unsafe { raw_eq(&[13_u8, 42], &[13, 42]) }; + assert!(RAW_EQ_ARRAY_TRUE); + + const RAW_EQ_ARRAY_FALSE: bool = unsafe { raw_eq(&[13_u8, 42], &[42, 13]) }; + assert!(!RAW_EQ_ARRAY_FALSE); +} From b63b2f1e426016b24a9e552faf17f20f421f5034 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 30 May 2021 11:31:56 -0700 Subject: [PATCH 3/9] PR feedback - Add `:Sized` assertion in interpreter impl - Use `Scalar::from_bool` instead of `ScalarInt: From` - Remove unneeded comparison in intrinsic typeck - Make this UB to call with undef, not just return undef in that case --- compiler/rustc_mir/src/interpret/intrinsics.rs | 3 ++- compiler/rustc_typeck/src/check/intrinsic.rs | 3 +-- library/core/src/intrinsics.rs | 8 ++++++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_mir/src/interpret/intrinsics.rs b/compiler/rustc_mir/src/interpret/intrinsics.rs index 5dd679b8912ce..ad9cf3e7d2fe9 100644 --- a/compiler/rustc_mir/src/interpret/intrinsics.rs +++ b/compiler/rustc_mir/src/interpret/intrinsics.rs @@ -570,11 +570,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { rhs: &OpTy<'tcx, >::PointerTag>, ) -> InterpResult<'tcx, Scalar> { let layout = self.layout_of(lhs.layout.ty.builtin_deref(true).unwrap().ty)?; + assert!(!layout.is_unsized()); let lhs = self.read_scalar(lhs)?.check_init()?; let rhs = self.read_scalar(rhs)?.check_init()?; let lhs_bytes = self.memory.read_bytes(lhs, layout.size)?; let rhs_bytes = self.memory.read_bytes(rhs, layout.size)?; - Ok(Scalar::Int((lhs_bytes == rhs_bytes).into())) + Ok(Scalar::from_bool(lhs_bytes == rhs_bytes)) } } diff --git a/compiler/rustc_typeck/src/check/intrinsic.rs b/compiler/rustc_typeck/src/check/intrinsic.rs index 18ccaf79d32c8..6661df21ed952 100644 --- a/compiler/rustc_typeck/src/check/intrinsic.rs +++ b/compiler/rustc_typeck/src/check/intrinsic.rs @@ -381,11 +381,10 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) { sym::nontemporal_store => (1, vec![tcx.mk_mut_ptr(param(0)), param(0)], tcx.mk_unit()), sym::raw_eq => { - let param_count = if intrinsic_name == sym::raw_eq { 2 } else { 1 }; let br = ty::BoundRegion { var: ty::BoundVar::from_u32(0), kind: ty::BrAnon(0) }; let param_ty = tcx.mk_imm_ref(tcx.mk_region(ty::ReLateBound(ty::INNERMOST, br)), param(0)); - (1, vec![param_ty; param_count], tcx.types.bool) + (1, vec![param_ty; 2], tcx.types.bool) } other => { diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index 7d2c278aa0523..238f00e41b3af 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -1924,8 +1924,12 @@ extern "rust-intrinsic" { /// /// # Safety /// - /// This doesn't take into account padding, so if `T` has padding - /// the result will be `undef`, which cannot be exposed to safe code. + /// It's UB to call this if any of the *bytes* in `*a` or `*b` are uninitialized. + /// Note that this is a stricter criterion than just the *values* being + /// fully-initialized: if `T` has padding, it's UB to call this intrinsic. + /// + /// (The implementation is allowed to branch on the results of comparisons, + /// which is UB if any of their inputs are `undef`.) #[cfg(not(bootstrap))] #[rustc_const_unstable(feature = "const_intrinsic_raw_eq", issue = "none")] pub fn raw_eq(a: &T, b: &T) -> bool; From 12163534a94b24a8a754fffe6e601904c2c6f31f Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 30 May 2021 18:04:07 -0700 Subject: [PATCH 4/9] Implement the raw_eq intrinsic in codegen_cranelift --- .../src/intrinsics/mod.rs | 39 +++++++++++++++++++ .../src/value_and_place.rs | 6 +++ 2 files changed, 45 insertions(+) diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index 52896fc7127e8..3e658cb121138 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs @@ -1115,6 +1115,45 @@ pub(crate) fn codegen_intrinsic_call<'tcx>( ); ret.write_cvalue(fx, CValue::by_val(res, ret.layout())); }; + + raw_eq, (v lhs_ref, v rhs_ref) { + fn type_by_size(size: Size) -> Option { + Some(match size.bits() { + 8 => types::I8, + 16 => types::I16, + 32 => types::I32, + 64 => types::I64, + 128 => types::I128, + _ => return None, + }) + } + + let size = fx.layout_of(T).layout.size; + let is_eq_value = + if size == Size::ZERO { + // No bytes means they're trivially equal + fx.bcx.ins().bconst(types::B1, true) + } else if let Some(clty) = type_by_size(size) { + // Can't use `trusted` for these loads; they could be unaligned. + let mut flags = MemFlags::new(); + flags.set_notrap(); + let lhs_val = fx.bcx.ins().load(clty, flags, lhs_ref, 0); + let rhs_val = fx.bcx.ins().load(clty, flags, rhs_ref, 0); + fx.bcx.ins().icmp(IntCC::Equal, lhs_val, rhs_val) + } else { + // Just call `memcmp` (like slices do in core) when the + // size is too large or it's not a power-of-two. + let ptr_ty = pointer_ty(fx.tcx); + let signed_bytes = i64::try_from(size.bytes()).unwrap(); + let bytes_val = fx.bcx.ins().iconst(ptr_ty, signed_bytes); + let params = vec![AbiParam::new(ptr_ty); 3]; + let returns = vec![AbiParam::new(types::I32)]; + let args = &[lhs_ref, rhs_ref, bytes_val]; + let cmp = fx.lib_call("memcmp", params, returns, args)[0]; + fx.bcx.ins().icmp_imm(IntCC::Equal, cmp, 0) + }; + ret.write_cvalue(fx, CValue::by_val(is_eq_value, ret.layout())); + }; } if let Some((_, dest)) = destination { diff --git a/compiler/rustc_codegen_cranelift/src/value_and_place.rs b/compiler/rustc_codegen_cranelift/src/value_and_place.rs index 171f39805f896..b6f5f5707fbc5 100644 --- a/compiler/rustc_codegen_cranelift/src/value_and_place.rs +++ b/compiler/rustc_codegen_cranelift/src/value_and_place.rs @@ -437,6 +437,12 @@ impl<'tcx> CPlace<'tcx> { | (types::F32, types::I32) | (types::I64, types::F64) | (types::F64, types::I64) => fx.bcx.ins().bitcast(dst_ty, data), + + // Widen an abstract SSA boolean to something that can be stored in memory + (types::B1, types::I8 | types::I16 | types::I32 | types::I64 | types::I128) => { + fx.bcx.ins().bint(dst_ty, data) + } + _ if src_ty.is_vector() && dst_ty.is_vector() => { fx.bcx.ins().raw_bitcast(dst_ty, data) } From 039a3bafecb42b51c2cc5f1bc1e0b0109873b729 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Sun, 30 May 2021 21:27:29 -0700 Subject: [PATCH 5/9] Add another codegen test, array_eq_zero Showing that this avoids an alloca and private constant. --- src/test/codegen/array-equality.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/test/codegen/array-equality.rs b/src/test/codegen/array-equality.rs index 6a9fb5c8f864d..aa56e32e26ce8 100644 --- a/src/test/codegen/array-equality.rs +++ b/src/test/codegen/array-equality.rs @@ -34,3 +34,12 @@ pub fn array_eq_long(a: &[u16; 1234], b: &[u16; 1234]) -> bool { // CHECK-NEXT: ret i1 %[[EQ]] a == b } + +// CHECK-LABEL: @array_eq_zero(i128 %0) +#[no_mangle] +pub fn array_eq_zero(x: [u16; 8]) -> bool { + // CHECK-NEXT: start: + // CHECK-NEXT: %[[EQ:.+]] = icmp eq i128 %0, 0 + // CHECK-NEXT: ret i1 %[[EQ]] + x == [0; 8] +} From 3d2869c6ff6ca7bd28383db6831f68b2d0349b0a Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Mon, 31 May 2021 10:26:08 -0700 Subject: [PATCH 6/9] PR Feedback: Don't put SSA-only types in `CValue`s --- compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs | 8 +++++--- .../rustc_codegen_cranelift/src/value_and_place.rs | 10 ++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index 3e658cb121138..31f7e0d4e37b1 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs @@ -1132,14 +1132,15 @@ pub(crate) fn codegen_intrinsic_call<'tcx>( let is_eq_value = if size == Size::ZERO { // No bytes means they're trivially equal - fx.bcx.ins().bconst(types::B1, true) + fx.bcx.ins().iconst(types::I8, 1) } else if let Some(clty) = type_by_size(size) { // Can't use `trusted` for these loads; they could be unaligned. let mut flags = MemFlags::new(); flags.set_notrap(); let lhs_val = fx.bcx.ins().load(clty, flags, lhs_ref, 0); let rhs_val = fx.bcx.ins().load(clty, flags, rhs_ref, 0); - fx.bcx.ins().icmp(IntCC::Equal, lhs_val, rhs_val) + let eq = fx.bcx.ins().icmp(IntCC::Equal, lhs_val, rhs_val); + fx.bcx.ins().bint(types::I8, eq) } else { // Just call `memcmp` (like slices do in core) when the // size is too large or it's not a power-of-two. @@ -1150,7 +1151,8 @@ pub(crate) fn codegen_intrinsic_call<'tcx>( let returns = vec![AbiParam::new(types::I32)]; let args = &[lhs_ref, rhs_ref, bytes_val]; let cmp = fx.lib_call("memcmp", params, returns, args)[0]; - fx.bcx.ins().icmp_imm(IntCC::Equal, cmp, 0) + let eq = fx.bcx.ins().icmp_imm(IntCC::Equal, cmp, 0); + fx.bcx.ins().bint(types::I8, eq) }; ret.write_cvalue(fx, CValue::by_val(is_eq_value, ret.layout())); }; diff --git a/compiler/rustc_codegen_cranelift/src/value_and_place.rs b/compiler/rustc_codegen_cranelift/src/value_and_place.rs index b6f5f5707fbc5..ae8ccc626b470 100644 --- a/compiler/rustc_codegen_cranelift/src/value_and_place.rs +++ b/compiler/rustc_codegen_cranelift/src/value_and_place.rs @@ -437,12 +437,6 @@ impl<'tcx> CPlace<'tcx> { | (types::F32, types::I32) | (types::I64, types::F64) | (types::F64, types::I64) => fx.bcx.ins().bitcast(dst_ty, data), - - // Widen an abstract SSA boolean to something that can be stored in memory - (types::B1, types::I8 | types::I16 | types::I32 | types::I64 | types::I128) => { - fx.bcx.ins().bint(dst_ty, data) - } - _ if src_ty.is_vector() && dst_ty.is_vector() => { fx.bcx.ins().raw_bitcast(dst_ty, data) } @@ -459,6 +453,10 @@ impl<'tcx> CPlace<'tcx> { ptr.store(fx, data, MemFlags::trusted()); ptr.load(fx, dst_ty, MemFlags::trusted()) } + + // `CValue`s should never contain SSA-only types, so if you ended + // up here having seen an error like `B1 -> I8`, then before + // calling `write_cvalue` you need to add a `bint` instruction. _ => unreachable!("write_cvalue_transmute: {:?} -> {:?}", src_ty, dst_ty), }; //fx.bcx.set_val_label(data, cranelift_codegen::ir::ValueLabel::new(var.index())); From 6444f24a29d1b9868e5dba647daf8209499757f6 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Tue, 1 Jun 2021 06:19:49 -0700 Subject: [PATCH 7/9] Use cranelift's `Type::int` instead of doing the match myself --- compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index 31f7e0d4e37b1..3979886e10cfc 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs @@ -1118,14 +1118,7 @@ pub(crate) fn codegen_intrinsic_call<'tcx>( raw_eq, (v lhs_ref, v rhs_ref) { fn type_by_size(size: Size) -> Option { - Some(match size.bits() { - 8 => types::I8, - 16 => types::I16, - 32 => types::I32, - 64 => types::I64, - 128 => types::I128, - _ => return None, - }) + Type::int(size.bits().try_into().ok()?) } let size = fx.layout_of(T).layout.size; From 07fb5ee78f4f251637c5c4414982a8c6e32e186d Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Wed, 2 Jun 2021 23:35:30 -0700 Subject: [PATCH 8/9] Adjust the threshold to look at the ABI, not just the size --- compiler/rustc_codegen_llvm/src/intrinsic.rs | 36 +++++++++++++------- src/test/codegen/array-equality.rs | 12 +++++++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/intrinsic.rs b/compiler/rustc_codegen_llvm/src/intrinsic.rs index 615295e96e116..9a968659e2fe8 100644 --- a/compiler/rustc_codegen_llvm/src/intrinsic.rs +++ b/compiler/rustc_codegen_llvm/src/intrinsic.rs @@ -297,28 +297,40 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> { } sym::raw_eq => { + use abi::Abi::*; let tp_ty = substs.type_at(0); - let (size, align) = self.size_and_align_of(tp_ty); + let layout = self.layout_of(tp_ty).layout; + let use_integer_compare = match layout.abi { + Scalar(_) | ScalarPair(_, _) => true, + Uninhabited | Vector { .. } => false, + Aggregate { .. } => { + // For rusty ABIs, small aggregates are actually passed + // as `RegKind::Integer` (see `FnAbi::adjust_for_abi`), + // so we re-use that same threshold here. + layout.size <= self.data_layout().pointer_size * 2 + } + }; + let a = args[0].immediate(); let b = args[1].immediate(); - if size.bytes() == 0 { + if layout.size.bytes() == 0 { self.const_bool(true) - } else if size > self.data_layout().pointer_size * 4 { + } else if use_integer_compare { + let integer_ty = self.type_ix(layout.size.bits()); + let ptr_ty = self.type_ptr_to(integer_ty); + let a_ptr = self.bitcast(a, ptr_ty); + let a_val = self.load(a_ptr, layout.align.abi); + let b_ptr = self.bitcast(b, ptr_ty); + let b_val = self.load(b_ptr, layout.align.abi); + self.icmp(IntPredicate::IntEQ, a_val, b_val) + } else { let i8p_ty = self.type_i8p(); let a_ptr = self.bitcast(a, i8p_ty); let b_ptr = self.bitcast(b, i8p_ty); - let n = self.const_usize(size.bytes()); + let n = self.const_usize(layout.size.bytes()); let llfn = self.get_intrinsic("memcmp"); let cmp = self.call(llfn, &[a_ptr, b_ptr, n], None); self.icmp(IntPredicate::IntEQ, cmp, self.const_i32(0)) - } else { - let integer_ty = self.type_ix(size.bits()); - let ptr_ty = self.type_ptr_to(integer_ty); - let a_ptr = self.bitcast(a, ptr_ty); - let a_val = self.load(a_ptr, align); - let b_ptr = self.bitcast(b, ptr_ty); - let b_val = self.load(b_ptr, align); - self.icmp(IntPredicate::IntEQ, a_val, b_val) } } diff --git a/src/test/codegen/array-equality.rs b/src/test/codegen/array-equality.rs index aa56e32e26ce8..4b60fa4b0bffa 100644 --- a/src/test/codegen/array-equality.rs +++ b/src/test/codegen/array-equality.rs @@ -23,6 +23,18 @@ pub fn array_eq_ref(a: &[u16; 6], b: &[u16; 6]) -> bool { a == b } +// CHECK-LABEL: @array_eq_value_still_passed_by_pointer +#[no_mangle] +pub fn array_eq_value_still_passed_by_pointer(a: [u16; 9], b: [u16; 9]) -> bool { + // CHECK-NEXT: start: + // CHECK-NEXT: bitcast + // CHECK-NEXT: bitcast + // CHECK-NEXT: %[[CMP:.+]] = tail call i32 @{{bcmp|memcmp}}(i8* nonnull dereferenceable(18) %{{.+}}, i8* nonnull dereferenceable(18) %{{.+}}, i64 18) + // CHECK-NEXT: %[[EQ:.+]] = icmp eq i32 %[[CMP]], 0 + // CHECK-NEXT: ret i1 %[[EQ]] + a == b +} + // CHECK-LABEL: @array_eq_long #[no_mangle] pub fn array_eq_long(a: &[u16; 1234], b: &[u16; 1234]) -> bool { From d0644947a3b093bfc09452ecd14291cea5dd8f14 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Thu, 8 Jul 2021 15:16:37 -0700 Subject: [PATCH 9/9] Bless a UI test --- .../intrinsic-raw_eq-const-padding.rs | 3 +-- .../intrinsic-raw_eq-const-padding.stderr | 20 ++++--------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.rs b/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.rs index ec1c47cfaea9f..a205a8730a0b8 100644 --- a/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.rs +++ b/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.rs @@ -4,8 +4,7 @@ const BAD_RAW_EQ_CALL: bool = unsafe { std::intrinsics::raw_eq(&(1_u8, 2_u16), &(1_u8, 2_u16)) -//~^ ERROR any use of this value will cause an error -//~| WARNING this was previously accepted by the compiler but is being phased out +//~^ ERROR evaluation of constant value failed }; pub fn main() { diff --git a/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.stderr b/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.stderr index 74df99a69d1fa..3a1b090012770 100644 --- a/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.stderr +++ b/src/test/ui/intrinsics/intrinsic-raw_eq-const-padding.stderr @@ -1,21 +1,9 @@ -error: any use of this value will cause an error +error[E0080]: evaluation of constant value failed --> $DIR/intrinsic-raw_eq-const-padding.rs:6:5 | -LL | / const BAD_RAW_EQ_CALL: bool = unsafe { -LL | | std::intrinsics::raw_eq(&(1_u8, 2_u16), &(1_u8, 2_u16)) - | | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reading 4 bytes of memory starting at alloc2, but 1 byte is uninitialized starting at alloc2+0x1, and this operation requires initialized memory -LL | | -LL | | -LL | | }; - | |__- - | -note: the lint level is defined here - --> $DIR/intrinsic-raw_eq-const-padding.rs:3:9 - | -LL | #![deny(const_err)] - | ^^^^^^^^^ - = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #71800 +LL | std::intrinsics::raw_eq(&(1_u8, 2_u16), &(1_u8, 2_u16)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reading 4 bytes of memory starting at alloc2, but 1 byte is uninitialized starting at alloc2+0x1, and this operation requires initialized memory error: aborting due to previous error +For more information about this error, try `rustc --explain E0080`.