Add invariant to Vec::pop that len < cap if pop successful

[krtab]

Fixes: #114334

[rustbot]

r? @Mark-Simulacrum

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

[mati865]

Without codegen test this is likely to regress at some point.

[the8472]

let's check the perf impact

@bors try @rust-timer queue

[bors]

⌛ Trying commit 32a0bf62062fcc99d927e2611eeefe2f62bf96f4 with merge 09bf8a0a3be933b7f221bdc99f8fdfe5e8a0306e...

[bors]

☀️ Try build successful - checks-actions
Build commit: 09bf8a0a3be933b7f221bdc99f8fdfe5e8a0306e (09bf8a0a3be933b7f221bdc99f8fdfe5e8a0306e)

[rust-timer]

Finished benchmarking commit (09bf8a0a3be933b7f221bdc99f8fdfe5e8a0306e): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.6%, 0.6%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	5.5%	[3.7%, 7.2%]	2
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-2.9%	[-3.0%, -2.8%]	2
Improvements ✅ (secondary)	-2.7%	[-3.4%, -2.0%]	2
All ❌✅ (primary)	1.3%	[-3.0%, 7.2%]	4

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.7%]	17
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.5%, -0.0%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.5%, 0.7%]	21

Bootstrap: 649.933s -> 651.065s (0.17%)

[scottmcm]

r? @scottmcm

I agree with @mati865 that this (like all assumes) should have a codegen test demonstrating that it enables some fruitful optimization that didn't happen without it.

@rustbot author

(That way when LLVM gets smarter or the code around it changes, people can try removing them again -- like in #111447 -- and the codegen test ensures that doesn't regress things.)

[krtab]

I've added a codegen test on the LLVM IR. I was unsure whether to add it as an LLVM-IR codegen or assembly codegen test. I chose the first because it felt more portable, but the second would have been closer to the spirit ("it should compile to a noop"). Let me know if you want me to change.

[scottmcm]

Hmm, the way this is written it feels a bit too specific, in that lots of little changes could make it fail (like bb3 getting a different name), but also that this tail call could be to something that reallocates and it'd pass.

Can you maybe focus it in a bit on what the critical part is?

Perhaps have another function in here showing that push normally can call __rust_realloc, but that this one doesn't?

A possible starting point:

#[no_mangle]
// CHECK-LABEL: @noop(
pub fn noop(v: &mut Vec<u8>) {
    // CHECK-NOT: __rust_alloc
    // CHECK-NOT: __rust_realloc
    // CHECK-NOT: call
    // CHECK: tail call void @llvm.assume
    // CHECK-NOT: __rust_alloc
    // CHECK-NOT: __rust_realloc
    // CHECK-NOT: call
    if let Some(x) = v.pop() {
        v.push(x)
    }
}

#[no_mangle]
// CHECK-LABEL: @push_byte(
pub fn push_byte(v: &mut Vec<u8>) {
    // CHECK: call {{.+}} @__rust_alloc(
    // CHECK: call {{.+}} @__rust_realloc(
    v.push(3);
}

[scottmcm]

Hmm, maybe it'll have to look at reserve_for_push instead of the allocation functions directly...

[krtab]

Hi,
I see your point but wouldn't it then be better to go for a codegen test on the assembly itself which is empty?

[scottmcm]

No, because everything should be codegen tests where possible -- random permutations of temporary assembly registers and differing calling conventions make assembly tests something that's a last resort.

[scottmcm]

@bors r+ rollup=never

[bors]

📌 Commit 0bcac8a has been approved by scottmcm

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0bcac8a with merge 49691b1...

[bors]

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 49691b1 to master...

[rust-timer]

Finished benchmarking commit (49691b1): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.4%, -0.4%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[1.3%, 1.8%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[1.3%, 1.8%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.2%]	20
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.9%, -0.0%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.9%, 0.2%]	24

Bootstrap: 626.726s -> 627.292s (0.09%)
Artifact size: 305.62 MiB -> 305.57 MiB (-0.02%)