Improved implementation and comments after code review feedback

6 files changed, 124 insertions, 77 deletions

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<T: Copy>(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<T>(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<T>(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<T>(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<u8>) -> MaybeUninit<u8> {
+    // 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>) -> 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<char>) -> MaybeUninit<char> {
+    // 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>) -> 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<u8> {
-    // 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<u8> {
-    // 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<u8>) -> MaybeUninit<u8> {
-    // 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<u8>) -> MaybeUninit<u8> {
-    // 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::<Never>}, val: Value(<ZST>) }
-+         _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
       }
   }