Simplify `slice::Iter::next` enough that it inlines #136771

scottmcm · 2025-02-09T11:00:05Z

Inspired by this zulip conversation: https://rust-lang.zulipchat.com/#narrow/channel/189540-t-compiler.2Fwg-mir-opt/topic/Feedback.20on.20a.20MIR.20optimization.20idea/near/498579990

~~Draft for now because it needs #136735 to get the codegen tests to pass.~~

rustbot · 2025-02-09T11:00:13Z

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

scottmcm · 2025-02-09T11:05:21Z

Let's see whether it actually improves things:
@bors try @rust-timer queue

bors · 2025-02-09T11:06:31Z

⌛ Trying commit f7970b3 with merge 30df00c...

Simplify `slice::Iter::next` enough that it inlines Inspired by this zulip conversation: <https://rust-lang.zulipchat.com/#narrow/channel/189540-t-compiler.2Fwg-mir-opt/topic/Feedback.20on.20a.20MIR.20optimization.20idea/near/498579990> Draft for now because it needs rust-lang#136735 to get the codegen tests to pass.

bors · 2025-02-09T12:58:41Z

☀️ Try build successful - checks-actions
Build commit: 30df00c (30df00cd8218095deb80cc6b913de02f5ae4a5b0)

rust-timer · 2025-02-09T15:26:04Z

Finished benchmarking commit (30df00c): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	9.4%	[9.4%, 9.4%]	1
Improvements ✅ (primary)	-0.6%	[-2.2%, -0.1%]	210
Improvements ✅ (secondary)	-0.5%	[-1.1%, -0.1%]	131
All ❌✅ (primary)	-0.6%	[-2.2%, 0.4%]	211

Max RSS (memory usage)

Results (primary -2.2%, secondary -2.5%)

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.4%	[1.7%, 2.8%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.4%	[-8.5%, -2.4%]	6
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-2.2%	[-8.5%, 2.8%]	9

Cycles

Results (primary -1.0%, secondary 1.7%)

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)	4.6%	[4.6%, 4.6%]	1
Improvements ✅ (primary)	-1.0%	[-1.1%, -0.9%]	2
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-1.0%	[-1.1%, -0.9%]	2

Binary size

Results (primary 0.1%, secondary -0.1%)

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.0%, 5.0%]	28
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.5%, -0.0%]	45
Improvements ✅ (secondary)	-0.1%	[-1.3%, -0.0%]	48
All ❌✅ (primary)	0.1%	[-0.5%, 5.0%]	73

Bootstrap: 780.488s -> 778.559s (-0.25%)
Artifact size: 329.04 MiB -> 329.26 MiB (0.06%)

scottmcm · 2025-02-11T09:53:21Z

tests/codegen/slice-iter-nonnull.rs

@@ -14,11 +14,11 @@
 // CHECK-LABEL: @slice_iter_next(
 #[no_mangle]
 pub fn slice_iter_next<'a>(it: &mut std::slice::Iter<'a, u32>) -> Option<&'a u32> {
-    // CHECK: %[[ENDP:.+]] = getelementptr inbounds{{( nuw)?}} i8, ptr %it, {{i32 4|i64 8}}
-    // CHECK: %[[END:.+]] = load ptr, ptr %[[ENDP]]
+    // CHECK: %[[START:.+]] = load ptr, ptr %it,


Rebased atop the transmute-gives-asserts change; codegen tests should be passing now with just this trivial change that it's loading the start pointer first instead of the end pointer first.

scottmcm · 2025-02-11T09:55:21Z

tests/mir-opt/pre-codegen/slice_iter.enumerated_loop.PreCodegen.after.panic-abort.mir

@@ -4,28 +4,30 @@ fn enumerated_loop(_1: &[T], _2: impl Fn(usize, &T)) -> () {
    debug slice => _1;
    debug f => _2;
    let mut _0: ();
-    let mut _11: std::slice::Iter<'_, T>;
-    let mut _12: std::iter::Enumerate<std::slice::Iter<'_, T>>;
-    let mut _13: std::iter::Enumerate<std::slice::Iter<'_, T>>;


Nice to see that the Enumerate iterators get completely SRoAed even just in MIR, with this!

scottmcm · 2025-02-11T09:56:09Z

Just to check that having the assumes in the LLVM-IR doesn't somehow lose all of the gains:
@bors try @rust-timer queue

bors · 2025-02-11T09:57:24Z

⌛ Trying commit 85deb4d with merge eaf73cd...

Simplify `slice::Iter::next` enough that it inlines Inspired by this zulip conversation: <https://rust-lang.zulipchat.com/#narrow/channel/189540-t-compiler.2Fwg-mir-opt/topic/Feedback.20on.20a.20MIR.20optimization.20idea/near/498579990> Draft for now because it needs rust-lang#136735 to get the codegen tests to pass.

bors · 2025-02-11T11:54:12Z

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

rust-timer · 2025-02-11T13:11:22Z

Finished benchmarking commit (eaf73cd): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.5%]	2
Regressions ❌ (secondary)	9.3%	[9.3%, 9.3%]	1
Improvements ✅ (primary)	-0.6%	[-2.2%, -0.1%]	196
Improvements ✅ (secondary)	-0.5%	[-2.4%, -0.1%]	101
All ❌✅ (primary)	-0.6%	[-2.2%, 0.5%]	198

Max RSS (memory usage)

Results (primary -1.8%, secondary -3.2%)

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.8%	[2.1%, 3.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-8.7%, -0.7%]	13
Improvements ✅ (secondary)	-3.2%	[-7.1%, -2.0%]	29
All ❌✅ (primary)	-1.8%	[-8.7%, 3.3%]	16

Cycles

Results (primary -1.4%, secondary 2.1%)

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)	3.0%	[2.1%, 5.4%]	5
Improvements ✅ (primary)	-1.4%	[-1.8%, -1.0%]	3
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-1.4%	[-1.8%, -1.0%]	3

Binary size

Results (primary 0.0%, secondary -0.2%)

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.0%, 3.9%]	38
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-1.8%, -0.1%]	50
Improvements ✅ (secondary)	-0.2%	[-2.0%, -0.0%]	83
All ❌✅ (primary)	0.0%	[-1.8%, 3.9%]	88

Bootstrap: 785.339s -> 785.217s (-0.02%)
Artifact size: 348.32 MiB -> 348.38 MiB (0.02%)

scottmcm · 2025-02-11T18:24:58Z

tests/mir-opt/pre-codegen/slice_iter.slice_iter_next.PreCodegen.after.panic-abort.mir

@@ -3,12 +3,134 @@
 fn slice_iter_next(_1: &mut std::slice::Iter<'_, T>) -> Option<&T> {
    debug it => _1;
    let mut _0: std::option::Option<&T>;
+    scope 1 (inlined <std::slice::Iter<'_, T> as Iterator>::next) {


If you want to compare to nightly, https://godbolt.org/z/Mr6cW9PWc

scottmcm · 2025-02-14T07:07:41Z

Make a small tweak to the approach; let's see whether it was actually good:
@bors try @rust-timer queue

bors · 2025-02-14T07:08:53Z

⌛ Trying commit 4ddbbbd with merge d786e76...

Simplify `slice::Iter::next` enough that it inlines Inspired by this zulip conversation: <https://rust-lang.zulipchat.com/#narrow/channel/189540-t-compiler.2Fwg-mir-opt/topic/Feedback.20on.20a.20MIR.20optimization.20idea/near/498579990> Draft for now because it needs rust-lang#136735 to get the codegen tests to pass.

bors · 2025-02-14T08:57:24Z

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

rust-timer · 2025-02-14T10:14:07Z

Finished benchmarking commit (d786e76): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	2
Regressions ❌ (secondary)	9.2%	[9.2%, 9.2%]	1
Improvements ✅ (primary)	-0.6%	[-2.2%, -0.2%]	204
Improvements ✅ (secondary)	-0.5%	[-2.3%, -0.1%]	101
All ❌✅ (primary)	-0.6%	[-2.2%, 0.5%]	206

Max RSS (memory usage)

Results (primary -1.7%, secondary 0.9%)

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)	6.4%	[2.9%, 9.8%]	2
Regressions ❌ (secondary)	3.8%	[3.4%, 4.2%]	2
Improvements ✅ (primary)	-3.5%	[-10.0%, -0.8%]	9
Improvements ✅ (secondary)	-5.0%	[-5.0%, -5.0%]	1
All ❌✅ (primary)	-1.7%	[-10.0%, 9.8%]	11

Cycles

Results (primary -1.2%, secondary 1.1%)

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)	4.5%	[4.5%, 4.5%]	1
Improvements ✅ (primary)	-1.2%	[-2.0%, -0.8%]	11
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-1.2%	[-2.0%, -0.8%]	11

Binary size

Results (primary -0.2%, secondary -0.1%)

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.3%	[0.0%, 0.8%]	31
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	34
Improvements ✅ (primary)	-0.4%	[-1.8%, -0.1%]	61
Improvements ✅ (secondary)	-0.3%	[-2.0%, -0.0%]	11
All ❌✅ (primary)	-0.2%	[-1.8%, 0.8%]	92

Bootstrap: 789.679s -> 787.912s (-0.22%)
Artifact size: 347.73 MiB -> 348.31 MiB (0.17%)

scottmcm · 2025-02-14T18:41:02Z

With #136735 having landed, this is good for a review now.

@rustbot ready

The one large regression is secondary, and looks to be one of the codegen units just having way more to do in LLVM:

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

the8472 · 2025-02-14T22:42:17Z

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

-                // safe since we check if the iterator is empty first.
+                let ptr = self.ptr;
+                let end_or_len = self.end_or_len;
+                // SAFETY: Type invariants.


The safety comment is a bit too sloppy imo. At least it should say something like "same as above" if you want to avoid repetition. Or maybe split it into two unsafe blocks, one for each arm.

Yeah, true.

Weirdly when I added tighter-scoped unsafe blocks it stopped inlining (and I even rebuilt to check because that's so strange), but I added more specific comments inside a bigger block.

…ffset` Probably reasonable anyway since it more obviously drops provenance.

This adds a few more statements to `next`, but optimizes better in the loops (saving 2 blocks in `forward_loop`, for example)

rustbot assigned joboet Feb 9, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 9, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 9, 2025

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 9, 2025

FractalFir mentioned this pull request Feb 9, 2025

[perf experiment] A MIR pass dedicated to optimizing common iterators #136745

Closed

scottmcm mentioned this pull request Feb 11, 2025

transmute should also assume non-null pointers #136735

Merged

scottmcm force-pushed the poke-slice-iter-next branch from f7970b3 to 85deb4d Compare February 11, 2025 09:51

scottmcm removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 11, 2025

scottmcm commented Feb 11, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 11, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 11, 2025

scottmcm commented Feb 11, 2025

View reviewed changes

scottmcm force-pushed the poke-slice-iter-next branch from 85deb4d to 4392415 Compare February 14, 2025 05:45

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 14, 2025

scottmcm closed this Feb 14, 2025

scottmcm reopened this Feb 14, 2025

scottmcm marked this pull request as ready for review February 14, 2025 18:40

the8472 reviewed Feb 14, 2025

View reviewed changes

library/core/src/slice/iter/macros.rs Show resolved Hide resolved

the8472 reviewed Feb 14, 2025

View reviewed changes

scottmcm added 4 commits February 14, 2025 22:24

Simplify slice::Iter::next enough that it inlines

aede8f5

Save another BB by using SubUnchecked instead of a call to `arith_o…

3a62c70

…ffset` Probably reasonable anyway since it more obviously drops provenance.

Go back to Some instead of transmuting to it.

39118d6

This adds a few more statements to `next`, but optimizes better in the loops (saving 2 blocks in `forward_loop`, for example)

Add real safety comments

7add358

scottmcm force-pushed the poke-slice-iter-next branch from 4ddbbbd to 7add358 Compare February 15, 2025 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify `slice::Iter::next` enough that it inlines #136771

Simplify `slice::Iter::next` enough that it inlines #136771

scottmcm commented Feb 9, 2025 •

edited

Loading

rustbot commented Feb 9, 2025

scottmcm commented Feb 9, 2025

This comment has been minimized.

bors commented Feb 9, 2025

This comment has been minimized.

bors commented Feb 9, 2025

This comment has been minimized.

rust-timer commented Feb 9, 2025

scottmcm Feb 11, 2025

scottmcm Feb 11, 2025

scottmcm commented Feb 11, 2025

This comment has been minimized.

bors commented Feb 11, 2025

bors commented Feb 11, 2025

This comment has been minimized.

rust-timer commented Feb 11, 2025

scottmcm Feb 11, 2025

scottmcm commented Feb 14, 2025

This comment has been minimized.

bors commented Feb 14, 2025

bors commented Feb 14, 2025

This comment has been minimized.

rust-timer commented Feb 14, 2025

scottmcm commented Feb 14, 2025 •

edited

Loading

the8472 Feb 14, 2025

scottmcm Feb 15, 2025

Simplify slice::Iter::next enough that it inlines #136771

Are you sure you want to change the base?

Simplify slice::Iter::next enough that it inlines #136771

Conversation

scottmcm commented Feb 9, 2025 • edited Loading

rustbot commented Feb 9, 2025

scottmcm commented Feb 9, 2025

This comment has been minimized.

bors commented Feb 9, 2025

This comment has been minimized.

bors commented Feb 9, 2025

This comment has been minimized.

rust-timer commented Feb 9, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Feb 11, 2025

This comment has been minimized.

bors commented Feb 11, 2025

bors commented Feb 11, 2025

This comment has been minimized.

rust-timer commented Feb 11, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Choose a reason for hiding this comment

scottmcm commented Feb 14, 2025

This comment has been minimized.

bors commented Feb 14, 2025

bors commented Feb 14, 2025

This comment has been minimized.

rust-timer commented Feb 14, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

scottmcm commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simplify `slice::Iter::next` enough that it inlines #136771

Simplify `slice::Iter::next` enough that it inlines #136771

scottmcm commented Feb 9, 2025 •

edited

Loading

scottmcm commented Feb 14, 2025 •

edited

Loading