Eliminate bounds checking in slice::Windows

[AnthonyMikh]

This is how <core::slice::Windows as Iterator>::next looks right now:

fnnext(&mutself) -> Option<&'a[T]>{ifself.size > self.v.len(){None}else{let ret = Some(&self.v[..self.size]);self.v = &self.v[1..];
ret
}}

The line with self.v = &self.v[1..]; relies on assumption that self.v is definitely not empty at this point. Else branch is taken when self.size <= self.v.len(), so self.v can be empty if self.size is zero. In practice, since Windows is never created directly but rather trough [T]::windows which panics when size is zero, self.size is never zero. However, the compiler doesn't know about this check, so it keeps the code which checks bounds and panics.

Using NonZeroUsize lets the compiler know about this invariant and reliably eliminate bounds checking without unsafe on -O2. Here is assembly of Windows<'a, u32>::next before and after this change (goldbolt):

Before

example::next:
push rax
mov rcx, qword ptr [rdi + 8]
mov rdx, qword ptr [rdi + 16]
cmp rdx, rcx
jbe .LBB0_2
xor eax, eax
pop rcx
ret
.LBB0_2:
test rcx, rcx
je .LBB0_5
mov rax, qword ptr [rdi]
mov rsi, rax
add rsi, 4
add rcx, -1
mov qword ptr [rdi], rsi
mov qword ptr [rdi + 8], rcx
pop rcx
ret
.LBB0_5:
lea rdx, [rip + .L__unnamed_1]
mov edi, 1
xor esi, esi
call qword ptr [rip + core::slice::slice_index_order_fail@GOTPCREL]
ud2
.L__unnamed_2:
.ascii "./example.rs"
.L__unnamed_1:
.quad .L__unnamed_2
.asciz "\f\000\000\000\000\000\000\000\016\000\000\000\027\000\000"

After

example::next:
mov rcx, qword ptr [rdi + 8]
mov rdx, qword ptr [rdi + 16]
cmp rdx, rcx
jbe .LBB0_2
xor eax, eax
ret
.LBB0_2:
mov rax, qword ptr [rdi]
lea rsi, [rax + 4]
add rcx, -1
mov qword ptr [rdi], rsi
mov qword ptr [rdi + 8], rcx
ret

Note the lack of call to core::slice::slice_index_order_fail in second snippet.

Possible reasons not to merge this PR:

• this changes the error message on panic in [T]::windows. However, AFAIK this messages are not covered by backwards compatibility policy.

[rust-highfive]

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

[lcnr]

Would you mind adding a codegen test which checks that windows().next() is unable to panic?

[lcnr]

I expect that you need something like this: https://github.com/bugadani/rust/blob/1d157ce797dddcee16a577796199b1144b4f7f34/src/test/codegen/enum-bounds-check-issue-13926.rs (potentially without min-llvm-version, depending on how stable this optimization is)

[AnthonyMikh]

Would you mind adding a codegen test which checks that windows().next() is unable to panic?

@lcnr I can add it. However, I see no way to run codegen check. How can I do it?

[lcnr]

./x.py test src/test/codegen/name_of_your_test.rs --stage 0 should work afaik

[AnthonyMikh]

@lcnr I've added test but I am not sure if it is correct. Or should I test Windows::next directly?

[lcnr]

seems good to me. Does this test fail without your changes?

[AnthonyMikh]

seems good to me. Does this test fail without your changes?

It... Passes without my changes.

Apparently I don't know how to write codegen tests

[lcnr]

Looks like you need slice_start_index_len_failhttps://rust.godbolt.org/z/eb3jfo

[AnthonyMikh]

@lcnr without my changes this codegen test fails on --stage 1 but passes with my changes. Thanks for directing.

[AnthonyMikh]

BTW I only now realised that my rather mechanical changes in other methods seem also to eliminate bounds check in next_back, last and nth_back. Should I make codegen tests for them as well?

[lcnr]

hmm, it probably won't hurt to use explicitly extend the test to also add functions for these methods 🤷 feel free to do so

r? @lcnr
otherwise r=me

[AnthonyMikh]

@lcnr I've added codegen tests for Windows methods which benefit from my changes. PR is ready to merge.

[lcnr]

Looking at the emitted code on https://godbolt.org/z/T716cx

It looks to me like next calls core::slice::index::slice_start_index_len_fail on the currently nightly and next_back calls core::slice::index::slice_end_index_len_fail rn.
Does next and next_back alone fail with the current nightly?

In general this looks to me like these tests might not be as useful as I thought, considering that method renames make them somewhat useless. Does it work to instead test with the following?

// CHECK-NOT: panic// CHECK-NOT: fail

[AnthonyMikh]

Indeed, it correctly checks with panic and fail as well.

[lcnr]

@bors r+

[bors]

📌 Commit e699e83 has been approved by lcnr

[tesuji]

Do we have guideline about what the phrase used in expect call ?

Suggested change

	let size = NonZeroUsize::new(size).expect("size is zero");
	let size = NonZeroUsize::new(size).expect("`size` cannot be zero");

[AnthonyMikh]

I am not sure, but before my change panic message was assertion failed: size != 0.

[tesuji]

this is bike-shredding so don't worry much about this.

[AnthonyMikh]

BTW while inspecting the assembly on order to determine which changed methods benefit from my changes I checked nth_back, and I was surprised to see the panicking functions still remain in resulting binary even with -C opt-level=3. After my changes nth_back looks like so:

// impl DoubleEndedIteator ...pubfnnth_back(&mutself,n:usize) -> Option<&'a[T]>{let(end, overflow) = self.v.len().overflowing_sub(n);if end < self.size.get() || overflow {self.v = &[];None}else{let ret = &self.v[end - self.size.get()..end];self.v = &self.v[..end - 1];Some(ret)}}

Here else branch is taken when both conditions in if are false. !overflow means that n <= self.v.len() and thus end <= self.v.len(). !(end < self.size.get()) means end >= self.size.get() (so end - self.size.get() is greater or equal to zero and doesn't underflow) and, since self.size is NonZeroUsize, by transitivity we get end > 0. These two invariants guarantees that both slicings of slice.v are performed in-bounds.

The compiler understands that end > 0 -- in fact, putting assert!(end > 0) in the beginning of else branch doesn't change assembly code at all. However, it fails to understand the first invariant -- putting assert!(end <= self.v.len()) generates assembly with panic-related code. Calling unsafe {core::intrinsics::assume(end <= self.v.len()); }doesn't elide panic-related code (though it merges two code paths to them). I believe that it can be called a compiler bug since both invariants can be established via data-flow analysis. Should I file an issue for this?

[scottmcm]

TBH I was surprised that NonZeroUSize was working for this, since it works reliably for layout optimizations but often doesn't work for code optimization, see #49572

[bors]

⌛ Testing commit e699e83 with merge 28928c7...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr
Pushing 28928c7 to master...