Auto merge of #122971 - scottmcm:slice-iter-provenance-v2, r=<try>

Make slice iterators carry only a single provenance

Today they carry two potentially-different provenances, which makes certain optimizations illegal at the LLVM-IR level.

In particular, it makes it matter whether an operation is done from the start pointer or the end pointer, since as far as LLVM knows those might have different provenance.

For example, this code
```rust
pub unsafe fn first_via_nth_back(mut it: std::slice::Iter<'_, i8>) -> &i8 {
    // CHECK: ret ptr %0
    let len = it.len();
    it.nth_back(len - 1).unwrap_unchecked()
}
```
is <https://rust.godbolt.org/z/8e61vqzhP>
```llvm
  %2 = ptrtoint ptr %1 to i64
  %3 = ptrtoint ptr %0 to i64
  %.neg = add i64 %3, 1
  %_6.neg = sub i64 %.neg, %2
  %_15.i6.i = getelementptr inbounds i8, ptr %1, i64 %_6.neg
  %_15.i.i = getelementptr inbounds i8, ptr %_15.i6.i, i64 -1
  ret ptr %_15.i.i
```
whereas after this PR it's just
```llvm
  ret ptr %0
```
(some `assume`s removed in both cases)

Author: bors

library/core/src/slice/iter.rs

@@ -11,8 +11,8 @@ use crate::iter::{
 use crate::marker::PhantomData;
 use crate::mem::{self, SizedTypeProperties};
 use crate::num::NonZero;
-use crate::ptr::{NonNull, without_provenance, without_provenance_mut};
-use crate::{cmp, fmt};
+use crate::ptr::{self, NonNull};
+use crate::{cmp, fmt, intrinsics};
 
 #[stable(feature = "boxed_slice_into_iter", since = "1.80.0")]
 impl<T> !Iterator for [T] {}
@@ -72,10 +72,14 @@ pub struct Iter<'a, T: 'a> {
     ///
     /// This address will be used for all ZST elements, never changed.
     ptr: NonNull<T>,
-    /// For non-ZSTs, the non-null pointer to the past-the-end element.
+    /// For non-ZSTs, the address of the past-the-end element.  This is
+    /// intentionally *not* a pointer, so that it doesn't carry provenance.
+    /// If you're turning this into a pointer, you need to use the provenance from
+    /// `ptr` instead.  (If this carried provenance, the compiler wouldn't know
+    /// that reads from the start and the end are actually the same provenance.)
     ///
-    /// For ZSTs, this is `ptr::without_provenance_mut(len)`.
-    end_or_len: *const T,
+    /// For ZSTs, this is the length.
+    end_addr_or_len: usize,
     _marker: PhantomData<&'a T>,
 }
 
@@ -98,10 +102,9 @@ impl<'a, T> Iter<'a, T> {
         let ptr: NonNull<T> = NonNull::from_ref(slice).cast();
         // SAFETY: Similar to `IterMut::new`.
         unsafe {
-            let end_or_len =
-                if T::IS_ZST { without_provenance(len) } else { ptr.as_ptr().add(len) };
+            let end_addr_or_len = if T::IS_ZST { len } else { addr_usize(ptr.add(len)) };
 
-            Self { ptr, end_or_len, _marker: PhantomData }
+            Self { ptr, end_addr_or_len, _marker: PhantomData }
         }
     }
 
@@ -153,7 +156,7 @@ iterator! {struct Iter -> *const T, &'a T, const, {/* no mut */}, as_ref, {
 impl<T> Clone for Iter<'_, T> {
     #[inline]
     fn clone(&self) -> Self {
-        Iter { ptr: self.ptr, end_or_len: self.end_or_len, _marker: self._marker }
+        Iter { ptr: self.ptr, end_addr_or_len: self.end_addr_or_len, _marker: self._marker }
     }
 }
 
@@ -197,10 +200,14 @@ pub struct IterMut<'a, T: 'a> {
     ///
     /// This address will be used for all ZST elements, never changed.
     ptr: NonNull<T>,
-    /// For non-ZSTs, the non-null pointer to the past-the-end element.
+    /// For non-ZSTs, the address of the past-the-end element.  This is
+    /// intentionally *not* a pointer, so that it doesn't carry provenance.
+    /// If you're turning this into a pointer, you need to use the provenance from
+    /// `ptr` instead.  (If this carried provenance, the compiler wouldn't know
+    /// that reads from the start and the end are actually the same provenance.)
     ///
-    /// For ZSTs, this is `ptr::without_provenance_mut(len)`.
-    end_or_len: *mut T,
+    /// For ZSTs, this is the length.
+    end_addr_or_len: usize,
     _marker: PhantomData<&'a mut T>,
 }
 
@@ -238,10 +245,9 @@ impl<'a, T> IterMut<'a, T> {
         // See the `next_unchecked!` and `is_empty!` macros as well as the
         // `post_inc_start` method for more information.
         unsafe {
-            let end_or_len =
-                if T::IS_ZST { without_provenance_mut(len) } else { ptr.as_ptr().add(len) };
+            let end_addr_or_len = if T::IS_ZST { len } else { addr_usize(ptr.add(len)) };
 
-            Self { ptr, end_or_len, _marker: PhantomData }
+            Self { ptr, end_addr_or_len, _marker: PhantomData }
         }
     }
 
@@ -3478,3 +3484,12 @@ impl<'a, T: 'a + fmt::Debug, P> fmt::Debug for ChunkByMut<'a, T, P> {
         f.debug_struct("ChunkByMut").field("slice", &self.slice).finish()
     }
 }
+
+/// Same as `p.addr().get()`, but faster to compile by avoiding a bunch of
+/// intermediate steps and unneeded UB checks, which also inlines better.
+#[inline]
+const fn addr_usize<T>(p: NonNull<T>) -> usize {
+    // SAFETY: `NonNull` for a sized type has the same layout as `usize`,
+    // and we intentionally don't want to expose here.
+    unsafe { intrinsics::transmute(p) }
+}

library/core/src/slice/iter/macros.rs

@@ -1,36 +1,64 @@
 //! Macros used by iterators of slice.
 
-/// Convenience & performance macro for consuming the `end_or_len` field, by
+/// Convenience macro for updating the `end_addr_or_len` field for non-ZSTs.
+macro_rules! set_end {
+    ($this:ident . end = $new_end:expr) => {{
+        $this.end_addr_or_len = addr_usize($new_end);
+    }};
+}
+
+/// Convenience & performance macro for consuming the `end_addr_or_len` field, by
 /// giving a `(&mut) usize` or `(&mut) NonNull<T>` depending whether `T` is
 /// or is not a ZST respectively.
 ///
-/// Internally, this reads the `end` through a pointer-to-`NonNull` so that
-/// it'll get the appropriate non-null metadata in the backend without needing
-/// to call `assume` manually.
+/// When giving a `NonNull<T>` for the end, it creates it by offsetting from the
+/// `ptr` so that the backend knows that both pointers have the same provenance.
 macro_rules! if_zst {
     (mut $this:ident, $len:ident => $zst_body:expr, $end:ident => $other_body:expr,) => {{
         #![allow(unused_unsafe)] // we're sometimes used within an unsafe block
 
+        let ptr = $this.ptr;
         if T::IS_ZST {
-            // SAFETY: for ZSTs, the pointer is storing a provenance-free length,
-            // so consuming and updating it as a `usize` is fine.
-            let $len = unsafe { &mut *(&raw mut $this.end_or_len).cast::<usize>() };
+            let $len = &mut $this.end_addr_or_len;
             $zst_body
         } else {
-            // SAFETY: for non-ZSTs, the type invariant ensures it cannot be null
-            let $end = unsafe { &mut *(&raw mut $this.end_or_len).cast::<NonNull<T>>() };
+            let $len;
+            #[allow(unused_unsafe)] // we're sometimes used within an unsafe block
+            // SAFETY: By type invariant `end >= ptr`, and thus the subtraction
+            // cannot overflow, and the iter represents a single allocated
+            // object so the `add` will also be in-range.
+            let $end = unsafe {
+                // Need to load as `NonNull` to get `!nonnull` metadata
+                // (Transmuting gets an assume instead)
+                let end: NonNull<T> = *ptr::addr_of!($this.end_addr_or_len).cast();
+                // Not using `with_addr` because we have ordering information that
+                // we can take advantage of here that `with_addr` cannot.
+                $len = intrinsics::ptr_offset_from_unsigned(end.as_ptr(), ptr.as_ptr());
+                ptr.add($len)
+            };
             $other_body
         }
     }};
     ($this:ident, $len:ident => $zst_body:expr, $end:ident => $other_body:expr,) => {{
-        #![allow(unused_unsafe)] // we're sometimes used within an unsafe block
-
         if T::IS_ZST {
-            let $len = $this.end_or_len.addr();
+            let $len = $this.end_addr_or_len;
             $zst_body
         } else {
-            // SAFETY: for non-ZSTs, the type invariant ensures it cannot be null
-            let $end = unsafe { mem::transmute::<*const T, NonNull<T>>($this.end_or_len) };
+            let $len;
+            #[allow(unused_unsafe)] // we're sometimes used within an unsafe block
+            // SAFETY: By type invariant `end >= ptr`, and thus the subtraction
+            // cannot overflow, and the iter represents a single allocated
+            // object so the `add` will also be in-range.
+            let $end = unsafe {
+                let ptr = $this.ptr;
+                // Need to load as `NonNull` to get `!nonnull` metadata
+                // (Transmuting gets an assume instead)
+                let end: NonNull<T> = *ptr::addr_of!($this.end_addr_or_len).cast();
+                // Not using `with_addr` because we have ordering information that
+                // we can take advantage of here that `with_addr` cannot.
+                $len = intrinsics::ptr_offset_from_unsigned(end.as_ptr(), ptr.as_ptr());
+                ptr.add($len)
+            };
             $other_body
         }
     }};
@@ -50,12 +78,7 @@ macro_rules! len {
     ($self: ident) => {{
         if_zst!($self,
             len => len,
-            end => {
-                // To get rid of some bounds checks (see `position`), we use ptr_sub instead of
-                // offset_from (Tested by `codegen/slice-position-bounds-check`.)
-                // SAFETY: by the type invariant pointers are aligned and `start <= end`
-                unsafe { end.offset_from_unsigned($self.ptr) }
-            },
+            _end => len,
         )
     }};
 }
@@ -128,8 +151,9 @@ macro_rules! iterator {
                     // which is guaranteed to not overflow an `isize`. Also, the resulting pointer
                     // is in bounds of `slice`, which fulfills the other requirements for `offset`.
                     end => unsafe {
-                        *end = end.sub(offset);
-                        *end
+                        let new_end = end.sub(offset);
+                        set_end!(self.end = new_end);
+                        new_end
                     },
                 )
             }
@@ -158,25 +182,24 @@ macro_rules! iterator {
                 // one of the most mono'd things in the library.
 
                 let ptr = self.ptr;
-                let end_or_len = self.end_or_len;
                 // SAFETY: See inner comments. (For some reason having multiple
                 // block breaks inlining this -- if you can fix that please do!)
                 unsafe {
                     if T::IS_ZST {
-                        let len = end_or_len.addr();
+                        let len = self.end_addr_or_len;
                         if len == 0 {
                             return None;
                         }
                         // SAFETY: just checked that it's not zero, so subtracting one
                         // cannot wrap.  (Ideally this would be `checked_sub`, which
                         // does the same thing internally, but as of 2025-02 that
                         // doesn't optimize quite as small in MIR.)
-                        self.end_or_len = without_provenance_mut(len.unchecked_sub(1));
+                        self.end_addr_or_len = intrinsics::unchecked_sub(len, 1);
                     } else {
-                        // SAFETY: by type invariant, the `end_or_len` field is always
+                        // SAFETY: by type invariant, the `end_addr_or_len` field is always
                         // non-null for a non-ZST pointee.  (This transmute ensures we
                         // get `!nonnull` metadata on the load of the field.)
-                        if ptr == crate::intrinsics::transmute::<$ptr, NonNull<T>>(end_or_len) {
+                        if ptr == *(&raw const self.end_addr_or_len).cast::<NonNull<T>>() {
                             return None;
                         }
                         // SAFETY: since it's not empty, per the check above, moving
@@ -207,7 +230,7 @@ macro_rules! iterator {
                     // This iterator is now empty.
                     if_zst!(mut self,
                         len => *len = 0,
-                        end => self.ptr = *end,
+                        end => self.ptr = end,
                     );
                     return None;
                 }
@@ -432,7 +455,7 @@ macro_rules! iterator {
                     // This iterator is now empty.
                     if_zst!(mut self,
                         len => *len = 0,
-                        end => *end = self.ptr,
+                        _end => set_end!(self.end = self.ptr),
                     );
                     return None;
                 }

tests/codegen/issues/issue-37945.rs

@@ -3,32 +3,33 @@
 
 // Check that LLVM understands that `Iter` pointer is not null. Issue #37945.
 
+// There used to be a comparison against `null`, so we check that it's not there
+// and that the appropriate parameter metadata is.
+
 #![crate_type = "lib"]
 
 use std::slice::Iter;
 
 #[no_mangle]
 pub fn is_empty_1(xs: Iter<f32>) -> bool {
-    // CHECK-LABEL: @is_empty_1(
-    // CHECK-NEXT:  start:
-    // CHECK-NEXT:    [[A:%.*]] = icmp ne ptr {{%xs.0|%xs.1}}, null
-    // CHECK-NEXT:    tail call void @llvm.assume(i1 [[A]])
-    // The order between %xs.0 and %xs.1 on the next line doesn't matter
-    // and different LLVM versions produce different order.
-    // CHECK-NEXT:    [[B:%.*]] = icmp eq ptr {{%xs.0, %xs.1|%xs.1, %xs.0}}
-    // CHECK-NEXT:    ret i1 [[B:%.*]]
+    // CHECK-LABEL: @is_empty_1
+    // CHECK-SAME: (ptr noundef nonnull{{.*}}%xs.0, {{i32|i64}}{{.+}}%xs.1)
+
+    // CHECK-NOT: null
+    // CHECK-NOT: i32 0
+    // CHECK-NOT: i64 0
+
     { xs }.next().is_none()
 }
 
 #[no_mangle]
 pub fn is_empty_2(xs: Iter<f32>) -> bool {
     // CHECK-LABEL: @is_empty_2
-    // CHECK-NEXT:  start:
-    // CHECK-NEXT:    [[C:%.*]] = icmp ne ptr {{%xs.0|%xs.1}}, null
-    // CHECK-NEXT:    tail call void @llvm.assume(i1 [[C]])
-    // The order between %xs.0 and %xs.1 on the next line doesn't matter
-    // and different LLVM versions produce different order.
-    // CHECK-NEXT:    [[D:%.*]] = icmp eq ptr {{%xs.0, %xs.1|%xs.1, %xs.0}}
-    // CHECK-NEXT:    ret i1 [[D:%.*]]
+    // CHECK-SAME: (ptr noundef nonnull{{.*}}%xs.0, {{i32|i64}}{{.+}}%xs.1)
+
+    // CHECK-NOT: null
+    // CHECK-NOT: i32 0
+    // CHECK-NOT: i64 0
+
     xs.map(|&x| x).next().is_none()
 }

tests/codegen/slice-iter-len-eq-zero.rs

@@ -1,4 +1,5 @@
 //@ compile-flags: -Copt-level=3
+//@ only-64bit
 //@ needs-deterministic-layouts (opposite scalar pair orders breaks it)
 #![crate_type = "lib"]
 
@@ -7,8 +8,11 @@ type Demo = [u8; 3];
 // CHECK-LABEL: @slice_iter_len_eq_zero
 #[no_mangle]
 pub fn slice_iter_len_eq_zero(y: std::slice::Iter<'_, Demo>) -> bool {
-    // CHECK-NOT: sub
-    // CHECK: %[[RET:.+]] = icmp eq ptr {{%y.0, %y.1|%y.1, %y.0}}
+    // CHECK: start:
+    // CHECK: %[[NOT_ZERO:.+]] = icmp ne i64 %1, 0
+    // CHECK: call void @llvm.assume(i1 %[[NOT_ZERO]])
+    // CHECK: %[[PTR_ADDR:.+]] = ptrtoint ptr %0 to i64
+    // CHECK: %[[RET:.+]] = icmp eq i64 %1, %[[PTR_ADDR]]
     // CHECK: ret i1 %[[RET]]
     y.len() == 0
 }
@@ -21,7 +25,7 @@ pub fn slice_iter_len_eq_zero_ref(y: &mut std::slice::Iter<'_, Demo>) -> bool {
     // CHECK-SAME: !nonnull
     // CHECK: %[[B:.+]] = load ptr
     // CHECK-SAME: !nonnull
-    // CHECK: %[[RET:.+]] = icmp eq ptr %[[A]], %[[B]]
+    // CHECK: %[[RET:.+]] = icmp eq ptr %[[B]], %[[A]]
     // CHECK: ret i1 %[[RET]]
     y.len() == 0
 }
@@ -31,17 +35,17 @@ struct MyZST;
 // CHECK-LABEL: @slice_zst_iter_len_eq_zero
 #[no_mangle]
 pub fn slice_zst_iter_len_eq_zero(y: std::slice::Iter<'_, MyZST>) -> bool {
-    // CHECK: %[[RET:.+]] = icmp eq ptr %y.1, null
+    // CHECK: %[[RET:.+]] = icmp eq i64 %y.1, 0
     // CHECK: ret i1 %[[RET]]
     y.len() == 0
 }
 
 // CHECK-LABEL: @slice_zst_iter_len_eq_zero_ref
 #[no_mangle]
 pub fn slice_zst_iter_len_eq_zero_ref(y: &mut std::slice::Iter<'_, MyZST>) -> bool {
-    // CHECK: %[[LEN:.+]] = load ptr
-    // CHECK-NOT: !nonnull
-    // CHECK: %[[RET:.+]] = icmp eq ptr %[[LEN]], null
+    // CHECK: %[[LEN:.+]] = load i64
+    // CHECK-NOT: !range
+    // CHECK: %[[RET:.+]] = icmp eq i64 %[[LEN]], 0
     // CHECK: ret i1 %[[RET]]
     y.len() == 0
 }