Remove the assume(!is_null) from Vec::as_ptr

[saethlin]

At a guess, this code is leftover from LLVM was worse at keeping track of the niche information here. In any case, we don't need this anymore: Removing this assume doesn't get rid of the nonnull attribute on the return type.

[rustbot]

r? @m-ou-se

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

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[saethlin]

@bors try

[bors]

⌛ Trying commit 328980eb5f134f896ca0aab39d4f44c1fda9d81e with merge c3ab48d9840510308050d3c3dc2879db69fad3b1...

[bors]

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

[saethlin]

@rust-timer build c3ab48d9840510308050d3c3dc2879db69fad3b1

[rust-timer]

Finished benchmarking commit (c3ab48d9840510308050d3c3dc2879db69fad3b1): comparison URL.

Overall result: ✅ improvements - 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
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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.2%	[-2.4%, -0.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.2%	[-2.4%, -0.1%]	3

Cycles

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

[saethlin]

MIR optimizations have changed substantially since the last run
@bors try @rust-timer queue

[bors]

⌛ Trying commit 328980eb5f134f896ca0aab39d4f44c1fda9d81e with merge c3e1d1d57310b9da0cafffacfe08242a9aad5858...

[saethlin]

Just realized that the same assume was in Vec::as_mut_ptr
@bors try @rust-timer queue

[bors]

⌛ Trying commit 738c8b0 with merge a61d19e671aa7efc184772d5a2e8172d172dcc0a...

[bors]

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

[rust-timer]

Finished benchmarking commit (a61d19e671aa7efc184772d5a2e8172d172dcc0a): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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)	1.1%	[0.4%, 2.2%]	6
Regressions ❌ (secondary)	0.6%	[0.4%, 1.2%]	11
Improvements ✅ (primary)	-1.4%	[-2.6%, -0.2%]	4
Improvements ✅ (secondary)	-0.6%	[-0.7%, -0.5%]	5
All ❌✅ (primary)	0.1%	[-2.6%, 2.2%]	10

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)	3.3%	[2.0%, 5.0%]	4
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-5.5%	[-6.6%, -3.3%]	3
Improvements ✅ (secondary)	-4.7%	[-4.7%, -4.7%]	1
All ❌✅ (primary)	-0.5%	[-6.6%, 5.0%]	7

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)	2.1%	[2.1%, 2.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.3%, -2.1%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-2.3%, 2.2%]	4

[saethlin]

It's been a little while, so
r? @thomcc

[thomcc]

Looks great

@bors r+

[bors]

📌 Commit 738c8b0 has been approved by thomcc

It is now in the queue for this repository.

[bors]

⌛ Testing commit 738c8b0 with merge ef03fda...

[bors]

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing ef03fda to master...

[bors]

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing ef03fda to master...

[rust-timer]

Finished benchmarking commit (ef03fda): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.3%, 1.7%]	16
Regressions ❌ (secondary)	0.7%	[0.2%, 1.4%]	15
Improvements ✅ (primary)	-0.8%	[-1.9%, -0.4%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-1.9%, 1.7%]	20

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%	[0.9%, 2.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.7%	[-7.0%, -0.5%]	7
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-7.0%, 2.3%]	10

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.5%	[0.4%, 0.7%]	5
Regressions ❌ (secondary)	13.4%	[12.8%, 14.2%]	6
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-1.6%, 0.7%]	6

[pnkfelix]

@RalfJung , as the person who added these assume calls back in PR #61114, do you have any comments on this?

this comment #61114 (comment) may be relevant, in terms of assume getting LLVM over-excited (and leading to wasted compilation time when it cannot actually leverage the calls)

[pnkfelix]

Visiting for rustc perf-triage

• results from final PR as it finally landed are a bit different (more broad in number of primary benchmarks effected) than the observations from trial performance run
• we expected to see [~2.0%, 2.2%] opt-full regression to regex and serde-derive
• we actually got a [~.7%, ~1.7%] regression to them instead; but a bunch of other primary benchmarks saw a slight dip (less than 0.7% of a regression for the new cases, many less than 0.4%)
• the cycle-counts and wall-times indicate more of an effect on clap and webrender here, but the effect there shows potentially more benefit than harm
• marking as triaged. Its too hard to make strong connections between the changes made here and actual costs/benefits.

@rustbot label: +perf-regression-triaged

[pnkfelix]

Incidentally, I do wonder how many of these assumes are technical artifact left over from PR #21886

[RalfJung]

as the person who added these assume calls back in PR #61114, do you have any comments on this?

I didn't "add" them, I copied them over from some other places... previously they were in the Deref/DerefMut impls for Vec. The concern about LLVM getting distracted by the assume was discussed in that PR but didn't reach a conclusion, so given that the assumes were already present before I just kept them in that PR.

Incidentally, I do wonder how many of these assumes are technical artifact left over from PR #21886

Seems pretty likely that the AsSlice instance was eventually turned into the Deref impl, and the assume carried over.

[saethlin]

FWIW, I'm almost certain that the changes here are related to MIR inlining, not the optimizations that LLVM is able to do as a result of the information that this assume provides it. The codegen test that this adds is meant to assuage any concerns about this. Based on the codegen test, it looks to me like LLVM is able to use the niche information in the NonNull type to derive the nonnull attribute on the resulting pointer. I would not be surprised if this was not the case 8+ years ago when this assume was added.

Vec::as{_mut}_ptr is called in a lot of places, and before this change it was quite large in MIR, which probably meant that in effect any code that used Vec didn't get a lot of MIR inlining. I suspect that perturbing MIR inlining like this will also perturb LLVM inlining. The existing use of #[inline(always)]/#[inline(never)] that is sprinkled over the compiler may have been overfit to the previous collective behavior of just LLVM inlining. I wouldn't be surprised if we could bring back some of the changes here (such as in the interpreter) with some strategic use of those attributes.