Auto merge of rust-lang#109035 - scottmcm:ptr-read-should-know-undef, r=WaffleLapkin,JakobDegen

Ensure `ptr::read` gets all the same LLVM `load` metadata that dereferencing does

I was looking into `array::IntoIter` optimization, and noticed that it wasn't annotating the loads with `noundef` for simple things like `array::IntoIter<i32, N>`.  Trying to narrow it down, it seems that was because `MaybeUninit::assume_init_read` isn't marking the load as initialized (<https://rust.godbolt.org/z/Mxd8TPTnv>), which is unfortunate since that's basically its reason to exist.

The root cause is that `ptr::read` is currently implemented via the *untyped* `copy_nonoverlapping`, and thus the `load` doesn't get any type-aware metadata: no `noundef`, no `!range`.  This PR solves that by lowering `ptr::read(p)` to `copy *p` in MIR, for which the backends already do the right thing.

Fortuitiously, this also improves the IR we give to LLVM for things like `mem::replace`, and fixes a couple of long-standing bugs where `ptr::read` on `Copy` types was worse than `*`ing them.

Zulip conversation: <https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Move.20array.3A.3AIntoIter.20to.20ManuallyDrop/near/341189936>

cc `@erikdesjardins` `@JakobDegen` `@workingjubilee` `@the8472`

Fixes rust-lang#106369
Fixes rust-lang#73258

Author: bors

core/src/intrinsics.rs

@@ -2020,6 +2020,16 @@ extern "rust-intrinsic" {
     #[rustc_safe_intrinsic]
     pub fn saturating_sub<T: Copy>(a: T, b: T) -> T;
 
+    /// This is an implementation detail of [`crate::ptr::read`] and should
+    /// not be used anywhere else.  See its comments for why this exists.
+    ///
+    /// 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;
+
     /// Returns the value of the discriminant for the variant in 'v';
     /// if `T` has no discriminant, returns `0`.
     ///

core/src/ptr/mod.rs

@@ -1135,27 +1135,58 @@ pub const unsafe fn replace<T>(dst: *mut T, mut src: T) -> T {
 #[rustc_const_unstable(feature = "const_ptr_read", issue = "80377")]
 #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
 pub const unsafe fn read<T>(src: *const T) -> T {
-    // We are calling the intrinsics directly to avoid function calls in the generated code
-    // as `intrinsics::copy_nonoverlapping` is a wrapper function.
-    extern "rust-intrinsic" {
-        #[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")]
-        fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize);
-    }
+    // It would be semantically correct to implement this via `copy_nonoverlapping`
+    // and `MaybeUninit`, as was done before PR #109035. Calling `assume_init`
+    // provides enough information to know that this is a typed operation.
 
-    let mut tmp = MaybeUninit::<T>::uninit();
-    // SAFETY: the caller must guarantee that `src` is valid for reads.
-    // `src` cannot overlap `tmp` because `tmp` was just allocated on
-    // the stack as a separate allocated object.
+    // 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:
     //
-    // Also, since we just wrote a valid value into `tmp`, it is guaranteed
-    // to be properly initialized.
+    // - Using `MaybeUninit::assume_init` after a `copy_nonoverlapping` was not
+    //   turning the untyped copy into a typed load. As such, the generated
+    //   `load` in LLVM didn't get various metadata, such as `!range` (#73258),
+    //   `!nonnull`, and `!noundef`, resulting in poorer optimization.
+    // - Going through the extra local resulted in multiple extra copies, even
+    //   in optimized MIR.  (Ignoring StorageLive/Dead, the intrinsic is one
+    //   MIR statement, while the previous implementation was eight.)  LLVM
+    //   could sometimes optimize them away, but because `read` is at the core
+    //   of so many things, not having them in the first place improves what we
+    //   hand off to the backend.  For example, `mem::replace::<Big>` previously
+    //   emitted 4 `alloca` and 6 `memcpy`s, but is now 1 `alloc` and 3 `memcpy`s.
+    // - In general, this approach keeps us from getting any more bugs (like
+    //   #106369) that boil down to "`read(p)` is worse than `*p`", as this
+    //   makes them look identical to the backend (or other MIR consumers).
+    //
+    // Future enhancements to MIR optimizations might well allow this to return
+    // to the previous implementation, rather than using an intrinsic.
+
+    // SAFETY: the caller must guarantee that `src` is valid for reads.
     unsafe {
         assert_unsafe_precondition!(
             "ptr::read requires that the pointer argument is aligned and non-null",
             [T](src: *const T) => is_aligned_and_not_null(src)
         );
-        copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
-        tmp.assume_init()
+
+        #[cfg(bootstrap)]
+        {
+            // We are calling the intrinsics directly to avoid function calls in the
+            // generated code as `intrinsics::copy_nonoverlapping` is a wrapper function.
+            extern "rust-intrinsic" {
+                #[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.63.0")]
+                fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize);
+            }
+
+            // `src` cannot overlap `tmp` because `tmp` was just allocated on
+            // the stack as a separate allocated object.
+            let mut tmp = MaybeUninit::<T>::uninit();
+            copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
+            tmp.assume_init()
+        }
+        #[cfg(not(bootstrap))]
+        {
+            crate::intrinsics::read_via_copy(src)
+        }
     }
 }