diff --git a/compiler/rustc_mir_transform/src/lower_intrinsics.rs b/compiler/rustc_mir_transform/src/lower_intrinsics.rs index 151fff27d14..5d7382305ae 100644 --- a/compiler/rustc_mir_transform/src/lower_intrinsics.rs +++ b/compiler/rustc_mir_transform/src/lower_intrinsics.rs @@ -150,7 +150,7 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics { } } sym::read_via_copy => { - let Ok([arg]) = <[_; 1]>::try_from(std::mem::take(args)) else { + let [arg] = args.as_slice() else { span_bug!(terminator.source_info.span, "Wrong number of arguments"); }; let derefed_place = @@ -159,18 +159,23 @@ impl<'tcx> MirPass<'tcx> for LowerIntrinsics { } else { span_bug!(terminator.source_info.span, "Only passing a local is supported"); }; - block.statements.push(Statement { - source_info: terminator.source_info, - kind: StatementKind::Assign(Box::new(( - *destination, - Rvalue::Use(Operand::Copy(derefed_place)), - ))), - }); - if let Some(target) = *target { - terminator.kind = TerminatorKind::Goto { target }; - } else { - // Reading something uninhabited means this is unreachable. - terminator.kind = TerminatorKind::Unreachable; + terminator.kind = match *target { + None => { + // No target means this read something uninhabited, + // so it must be unreachable, and we don't need to + // preserve the assignment either. + TerminatorKind::Unreachable + } + Some(target) => { + block.statements.push(Statement { + source_info: terminator.source_info, + kind: StatementKind::Assign(Box::new(( + *destination, + Rvalue::Use(Operand::Copy(derefed_place)), + ))), + }); + TerminatorKind::Goto { target } + } } } sym::discriminant_value => { diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index d77c2a00eb7..ee8846675ce 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -2020,16 +2020,12 @@ extern "rust-intrinsic" { #[rustc_safe_intrinsic] pub fn saturating_sub(a: T, b: T) -> T; - /// This is a *typed* read, `copy *p` in MIR. + /// This is an implementation detail of [`crate::ptr::read`] and should + /// not be used anywhere else. See its comments for why this exists. /// - /// The stabilized form of this intrinsic is [`crate::ptr::read`], so - /// that can be implemented without needing to do an *untyped* copy - /// via [`copy_nonoverlapping`], and thus can get proper metadata. - /// - /// This intrinsic can *only* be called with a copy or move of a local. - /// (It allows neither constants nor projections.) - /// - /// To avoid introducing any `noalias` requirements, it just takes a pointer. + /// This intrinsic can *only* be called where the argument is a local without + /// projections (`read_via_copy(p)`, not `read_via_copy(*p)`) so that it + /// trivially obeys runtime-MIR rules about derefs in operands. #[cfg(not(bootstrap))] #[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")] pub fn read_via_copy(p: *const T) -> T; diff --git a/library/core/src/ptr/mod.rs b/library/core/src/ptr/mod.rs index 86929e2c488..5884a8ca308 100644 --- a/library/core/src/ptr/mod.rs +++ b/library/core/src/ptr/mod.rs @@ -1136,10 +1136,12 @@ pub const unsafe fn replace(dst: *mut T, mut src: T) -> T { #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces pub const unsafe fn read(src: *const T) -> T { // It would be semantically correct to implement this via `copy_nonoverlapping` - // and `MaybeUninit`, as was done before PR #109035. + // and `MaybeUninit`, as was done before PR #109035. Calling `assume_init` + // provides enough information to know that this is a typed operation. - // However, it switched to intrinsic that lowers to `_0 = *src` in MIR in - // order to address a few implementation issues: + // However, as of March 2023 the compiler was not capable of taking advantage + // of that information. Thus the implementation here switched to an intrinsic, + // which lowers to `_0 = *src` in MIR, to address a few issues: // // - Using `MaybeUninit::assume_init` after a `copy_nonoverlapping` was not // turning the untyped copy into a typed load. As such, the generated diff --git a/tests/codegen/ptr-read-metadata.rs b/tests/codegen/ptr-read-metadata.rs new file mode 100644 index 00000000000..e1e3272662c --- /dev/null +++ b/tests/codegen/ptr-read-metadata.rs @@ -0,0 +1,96 @@ +// compile-flags: -O -Z merge-functions=disabled +// no-system-llvm +// ignore-debug (the extra assertions get in the way) + +#![crate_type = "lib"] + +// Ensure that various forms of reading pointers correctly annotate the `load`s +// with `!noundef` and `!range` metadata to enable extra optimization. + +use std::mem::MaybeUninit; + +// CHECK-LABEL: define noundef i8 @copy_byte( +#[no_mangle] +pub unsafe fn copy_byte(p: *const u8) -> u8 { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + *p +} + +// CHECK-LABEL: define noundef i8 @read_byte( +#[no_mangle] +pub unsafe fn read_byte(p: *const u8) -> u8 { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define i8 @read_byte_maybe_uninit( +#[no_mangle] +pub unsafe fn read_byte_maybe_uninit(p: *const MaybeUninit) -> MaybeUninit { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-NOT: noundef + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define noundef i8 @read_byte_assume_init( +#[no_mangle] +pub unsafe fn read_byte_assume_init(p: &MaybeUninit) -> u8 { + // CHECK-NOT: load + // CHECK: load i8, ptr %p, align 1 + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + p.assume_init_read() +} + +// CHECK-LABEL: define noundef i32 @copy_char( +#[no_mangle] +pub unsafe fn copy_char(p: *const char) -> char { + // CHECK-NOT: load + // CHECK: load i32, ptr %p + // CHECK-SAME: !range ![[RANGE:[0-9]+]] + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + *p +} + +// CHECK-LABEL: define noundef i32 @read_char( +#[no_mangle] +pub unsafe fn read_char(p: *const char) -> char { + // CHECK-NOT: load + // CHECK: load i32, ptr %p + // CHECK-SAME: !range ![[RANGE]] + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define i32 @read_char_maybe_uninit( +#[no_mangle] +pub unsafe fn read_char_maybe_uninit(p: *const MaybeUninit) -> MaybeUninit { + // CHECK-NOT: load + // CHECK: load i32, ptr %p + // CHECK-NOT: range + // CHECK-NOT: noundef + // CHECK-NOT: load + p.read() +} + +// CHECK-LABEL: define noundef i32 @read_char_assume_init( +#[no_mangle] +pub unsafe fn read_char_assume_init(p: &MaybeUninit) -> char { + // CHECK-NOT: load + // CHECK: load i32, ptr %p + // CHECK-SAME: !range ![[RANGE]] + // CHECK-SAME: !noundef ! + // CHECK-NOT: load + p.assume_init_read() +} + +// CHECK: ![[RANGE]] = !{i32 0, i32 1114112} diff --git a/tests/codegen/read-noundef-metadata.rs b/tests/codegen/read-noundef-metadata.rs deleted file mode 100644 index 03386921c43..00000000000 --- a/tests/codegen/read-noundef-metadata.rs +++ /dev/null @@ -1,51 +0,0 @@ -// compile-flags: -O -Z merge-functions=disabled -// no-system-llvm -// ignore-debug (the extra assertions get in the way) - -#![crate_type = "lib"] - -// Ensure that various forms of reading pointers correctly annotate the `load`s -// with `!noundef` metadata to enable extra optimization. The functions return -// `MaybeUninit` to keep it from being inferred from the function type. - -use std::mem::MaybeUninit; - -// CHECK-LABEL: define i8 @copy_byte( -#[no_mangle] -pub unsafe fn copy_byte(p: *const u8) -> MaybeUninit { - // CHECK-NOT: load - // CHECK: load i8, ptr %p, align 1 - // CHECK-SAME: !noundef ! - // CHECK-NOT: load - MaybeUninit::new(*p) -} - -// CHECK-LABEL: define i8 @read_byte( -#[no_mangle] -pub unsafe fn read_byte(p: *const u8) -> MaybeUninit { - // CHECK-NOT: load - // CHECK: load i8, ptr %p, align 1 - // CHECK-SAME: !noundef ! - // CHECK-NOT: load - MaybeUninit::new(p.read()) -} - -// CHECK-LABEL: define i8 @read_byte_maybe_uninit( -#[no_mangle] -pub unsafe fn read_byte_maybe_uninit(p: *const MaybeUninit) -> MaybeUninit { - // CHECK-NOT: load - // CHECK: load i8, ptr %p, align 1 - // CHECK-NOT: noundef - // CHECK-NOT: load - p.read() -} - -// CHECK-LABEL: define i8 @read_byte_assume_init( -#[no_mangle] -pub unsafe fn read_byte_assume_init(p: &MaybeUninit) -> MaybeUninit { - // CHECK-NOT: load - // CHECK: load i8, ptr %p, align 1 - // CHECK-SAME: !noundef ! - // CHECK-NOT: load - MaybeUninit::new(p.assume_init_read()) -} diff --git a/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff b/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff index faf3f9c31b8..610c67d2fec 100644 --- a/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff +++ b/tests/mir-opt/lower_intrinsics.read_via_copy_uninhabited.LowerIntrinsics.diff @@ -15,7 +15,6 @@ - // mir::Constant - // + span: $DIR/lower_intrinsics.rs:90:14: 90:45 - // + literal: Const { ty: unsafe extern "rust-intrinsic" fn(*const Never) -> Never {read_via_copy::}, val: Value() } -+ _0 = (*_2); // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 + unreachable; // scope 1 at $DIR/lower_intrinsics.rs:+1:14: +1:48 } }