Make slice iterators carry only a single provenance #122971
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,36 +1,60 @@ | ||
| //! Macros used by iterators of slice. | ||
|
|
||
| /// 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. | ||
| /// | ||
| /// 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 | ||
|
|
||
| if T::IS_ZST { | ||
| let $len = &mut $this.end_addr_or_len; | ||
| $zst_body | ||
| } else { | ||
| // 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_addr = addr_usize($this.ptr); | ||
| // Need to load as `NonZero` to get `!range` metadata | ||
| let end_addr: NonZero<usize> = *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. | ||
| let byte_diff = intrinsics::unchecked_sub(end_addr.get(), ptr_addr); | ||
| $this.ptr.byte_add(byte_diff) | ||
| }; | ||
|
Comment on lines
+24
to
+35
There was a problem hiding this comment. That's unfortunate. I think this will make #120682 impossible since there my approach for forward iteration is to only operate off It'd be great if instead we could somehow tell LLVM that two pointers have the same provenance. There was a problem hiding this comment. Could we keep it a pointer so it can be used as such when it's useful and do ptr-addr-arithmetic-ptr dances otherwise? or would that lose the intended benefits? There was a problem hiding this comment. I think the problem with "when it's useful" is that it has to mean potentially-two-provenances, so There was a problem hiding this comment. My goal with that PR is to make unchecked indexing less hazardous and also applicable to Also, have you checked how this affects looping over
For those methods. But the other methods could treat the field as usize, no? |
||
| $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_addr_or_len; | ||
| $zst_body | ||
| } else { | ||
| // 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_addr = addr_usize($this.ptr); | ||
| // Need to load as `NonZero` to get `!range` metadata | ||
| let end_addr: NonZero<usize> = *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. | ||
| let byte_diff = intrinsics::unchecked_sub(end_addr.get(), ptr_addr); | ||
| $this.ptr.byte_add(byte_diff) | ||
| }; | ||
| $other_body | ||
| } | ||
| }}; | ||
|
|
@@ -128,8 +152,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 { | ||
| let new_end = end.sub(offset); | ||
| set_end!(self.end = new_end); | ||
| new_end | ||
| }, | ||
| ) | ||
| } | ||
|
|
@@ -184,7 +209,7 @@ macro_rules! iterator { | |
| // This iterator is now empty. | ||
| if_zst!(mut self, | ||
| len => *len = 0, | ||
| end => self.ptr = end, | ||
| ); | ||
| return None; | ||
| } | ||
|
|
@@ -409,7 +434,7 @@ macro_rules! iterator { | |
| // This iterator is now empty. | ||
| if_zst!(mut self, | ||
| len => *len = 0, | ||
| _end => set_end!(self.end = self.ptr), | ||
| ); | ||
| return None; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case where the existence of this method can be made unnecessary? It feels weird that the alternative would perform so poorly, or involve UB checks at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runtime performance of
.addr().get()is completely fine.The problem is just in the sheer volume of MIR that it ends up producing -- just transmuting directly is literally an order of magnitude less stuff: https://rust.godbolt.org/z/cnzTW51oh
And with how often these are used, that makes a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I can remove a bit of that, at least: #123139
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm mostly just wondering if this is sufficient motivation to add this method on
NonNulldirectly (it can bepub(crate)for now), whether this hack is in fact specific to this module, or whether this function should be marked as FIXME and should be addressed later.