Add `SingleUseConsts` mir-opt pass #125910

scottmcm · 2024-06-03T02:41:48Z

The goal here is to make a pass that can be run in debug builds to simplify the common case of constants that are used just once -- that doesn't need SSA handling and avoids any potential downside of multi-use constants. In particular, to simplify the if T::IS_ZST pattern that's common in the standard library.

By also handling the case of constants that are never actually used this fully replaces the ConstDebugInfo pass, since it has all the information needed to do that naturally from the traversal it needs to do anyway.

This is roughly a wash on instructions on its own (a couple regressions, a few improvements #125910 (comment)), with a bunch of size improvements. So I'd like to land it as its own PR, then do follow-ups to take more advantage of it (in the inliner, cg_ssa, etc).

r? @saethlin

scottmcm · 2024-06-03T04:52:39Z

@bors try @rust-timer queue

bors · 2024-06-03T04:53:51Z

⌛ Trying commit fea5299 with merge d1611ef...

Add `SingleUseConsts` mir-opt pass r? ghost

scottmcm · 2024-06-03T05:49:56Z

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

-        _7 = const <T as std::mem::SizedTypeProperties>::IS_ZST;
-        switchInt(move _7) -> [0: bb1, otherwise: bb2];
+        StorageLive(_9);
+        switchInt(const <T as std::mem::SizedTypeProperties>::IS_ZST) -> [0: bb1, otherwise: bb2];


This is the main impetus for this change. By doing this rewrite, it'll be easier for other things to see that it's a switch on a constant. For example, the inliner wants to lower the cost of inlining such a branch, and codegen wants to use it to emit a goto instead of a switch, even in debug.

bors · 2024-06-03T06:31:35Z

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

rust-timer · 2024-06-03T08:34:52Z

Finished benchmarking commit (d1611ef): 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)	0.6%	[0.3%, 1.0%]	2
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	4
Improvements ✅ (secondary)	-0.8%	[-1.7%, -0.3%]	3
All ❌✅ (primary)	0.0%	[-0.4%, 1.0%]	6

Max RSS (memory usage)

Results (primary -3.9%, secondary -2.6%)

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.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	4.3%	[4.3%, 4.3%]	1
Improvements ✅ (primary)	-5.5%	[-9.2%, -2.1%]	3
Improvements ✅ (secondary)	-3.9%	[-5.2%, -2.8%]	5
All ❌✅ (primary)	-3.9%	[-9.2%, 0.9%]	4

Cycles

Results (primary -0.8%, secondary -1.6%)

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)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-1.6%	[-1.6%, -1.6%]	1
All ❌✅ (primary)	-0.8%	[-0.8%, -0.8%]	1

Binary size

Results (primary -0.2%, secondary -0.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)	0.1%	[0.0%, 0.3%]	24
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-1.3%, -0.0%]	62
Improvements ✅ (secondary)	-0.5%	[-1.2%, -0.3%]	14
All ❌✅ (primary)	-0.2%	[-1.3%, 0.3%]	86

Bootstrap: 667.872s -> 669.455s (0.24%)
Artifact size: 318.92 MiB -> 318.90 MiB (-0.00%)

scottmcm · 2024-06-03T08:37:27Z

@bors try @rust-timer queue

bors · 2024-06-03T08:38:37Z

⌛ Trying commit d543a8e with merge 620bf99...

Add `SingleUseConsts` mir-opt pass r? ghost

bors · 2024-06-03T10:17:13Z

☀️ Try build successful - checks-actions
Build commit: 620bf99 (620bf99590ace4816a30bbe23ef58605e353214b)

rust-timer · 2024-06-03T11:32:41Z

Finished benchmarking commit (620bf99): 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)	0.6%	[0.3%, 0.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	5
Improvements ✅ (secondary)	-0.8%	[-1.7%, -0.3%]	3
All ❌✅ (primary)	-0.1%	[-0.4%, 0.9%]	7

Max RSS (memory usage)

Results (primary -3.5%, secondary -3.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)	0.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	5.5%	[5.5%, 5.5%]	1
Improvements ✅ (primary)	-4.6%	[-7.0%, -2.1%]	4
Improvements ✅ (secondary)	-4.5%	[-6.1%, -2.6%]	9
All ❌✅ (primary)	-3.5%	[-7.0%, 0.9%]	5

Cycles

Results (primary 0.9%, secondary -0.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)	0.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.5%, -1.7%]	2
All ❌✅ (primary)	0.9%	[0.9%, 0.9%]	1

Binary size

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

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.3%]	24
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-1.4%, -0.0%]	62
Improvements ✅ (secondary)	-0.6%	[-1.3%, -0.3%]	14
All ❌✅ (primary)	-0.2%	[-1.4%, 0.3%]	86

Bootstrap: 670.518s -> 671.554s (0.15%)
Artifact size: 318.94 MiB -> 318.90 MiB (-0.01%)

rustbot · 2024-06-03T16:24:15Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

saethlin · 2024-06-10T01:16:08Z

compiler/rustc_mir_transform/src/single_use_consts.rs

+            let StatementKind::Assign(place_and_rvalue) = init_statement.kind else {
+                bug!("No longer an assign?");
+            };
+            let (place, rvalue) = *place_and_rvalue;


Are you deliberately avoiding using a box pattern?

I never even thought about using one -- my mental "how do I write a pattern for this" doesn't use the unstable stuff.

Since this is infallible, my instinct is to not use it. When it makes the control flow way easier in a refutable pattern then sure I'd say to use a box pattern, but here that's not the case.

Mm I meant if you use a box pattern you can combine these two lines. As far as I'm aware, a box pattern is the only way to do so.

But if you don't want to that's fine. I'm just used to seeing box patterns used for matching on StatementKind::Assign.

saethlin · 2024-06-10T01:16:10Z

compiler/rustc_mir_transform/src/single_use_consts.rs

+                bug!("No longer an assign?");
+            };
+            let (place, rvalue) = *place_and_rvalue;
+            debug_assert_eq!(place.as_local(), Some(local));


Is there a perf argument for enabling this only in debug builds? As a general rule, I prefer to have such checks either on for everyone. If a check for some absurd circumstance is going to trip, it seems more likely to trip in the wild than in CI.

I don't have a perf argument for it. I wrote it more as documentation than expecting it to ever fire -- a "this thing we just popped was the assignment to the local".

I can make it an assert_eq if you want 🤷

Ok, changed to assert_eq!.

saethlin · 2024-06-10T01:17:16Z

this fully replaces the ConstDebugInfo pass,

So why haven't we deleted the pass? I know we have somewhat of a tradition of keeping MIR transforms that are off by default or broken in the tree and I think that's extremely dubious. Especially in this case. What value will someone get from ConstDebugInfo at -Zmir-opt-level=3?

scottmcm · 2024-06-10T07:07:19Z

So why haven't we deleted the pass?

Added a new commit that deletes it (and rebased for tidy).

saethlin · 2024-06-11T00:11:45Z

Thanks!
@bors r+

bors · 2024-06-11T00:11:48Z

📌 Commit 8fbab18 has been approved by saethlin

It is now in the queue for this repository.

bors · 2024-06-11T02:03:16Z

⌛ Testing commit 8fbab18 with merge fa1681c...

bors · 2024-06-11T04:13:51Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing fa1681c to master...

rust-timer · 2024-06-11T05:30:23Z

Finished benchmarking commit (fa1681c): 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.4%	[0.3%, 0.4%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	4
Improvements ✅ (secondary)	-2.3%	[-2.9%, -1.7%]	2
All ❌✅ (primary)	-0.1%	[-0.4%, 0.4%]	6

Max RSS (memory usage)

Results (primary -0.2%, secondary 5.6%)

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.4%	[2.9%, 4.4%]	3
Regressions ❌ (secondary)	5.6%	[5.6%, 5.6%]	1
Improvements ✅ (primary)	-2.9%	[-4.1%, -2.3%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-4.1%, 4.4%]	7

Cycles

Results (secondary -5.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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.2%	[-7.0%, -1.9%]	6
All ❌✅ (primary)	-	-	0

Binary size

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

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.4%]	19
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-1.0%, -0.0%]	66
Improvements ✅ (secondary)	-0.6%	[-1.3%, -0.3%]	14
All ❌✅ (primary)	-0.1%	[-1.0%, 0.4%]	85

Bootstrap: 673.568s -> 675.712s (0.32%)
Artifact size: 320.09 MiB -> 320.06 MiB (-0.01%)

Add `SingleUseConsts` mir-opt pass The goal here is to make a pass that can be run in debug builds to simplify the common case of constants that are used just once -- that doesn't need SSA handling and avoids any potential downside of multi-use constants. In particular, to simplify the `if T::IS_ZST` pattern that's common in the standard library. By also handling the case of constants that are *never* actually used this fully replaces the `ConstDebugInfo` pass, since it has all the information needed to do that naturally from the traversal it needs to do anyway. This is roughly a wash on instructions on its own (a couple regressions, a few improvements rust-lang/rust#125910 (comment)), with a bunch of size improvements. So I'd like to land it as its own PR, then do follow-ups to take more advantage of it (in the inliner, cg_ssa, etc). r? `@saethlin`

Kobzol · 2024-06-11T07:21:10Z

Wash on instruction counts, but nice binary size wins. The cycles improvements on tt-muncher is noise returning back to normal.

@rustbot label: +perf-regression-triaged

Related changes: - rust-lang/rust#125910: Introduces a new constant propagation pass which broke Kani coverage tests. For now, disable this pass if coverage is enabled. - rust-lang/rust#126410: Rename ConstOperands

Related changes: - rust-lang/rust#125910: Introduces a new constant propagation pass which broke Kani coverage tests. For now, disable this pass if coverage is enabled. - rust-lang/rust#126410: Rename ConstOperands Resolves #3260

Add `SingleUseConsts` mir-opt pass The goal here is to make a pass that can be run in debug builds to simplify the common case of constants that are used just once -- that doesn't need SSA handling and avoids any potential downside of multi-use constants. In particular, to simplify the `if T::IS_ZST` pattern that's common in the standard library. By also handling the case of constants that are *never* actually used this fully replaces the `ConstDebugInfo` pass, since it has all the information needed to do that naturally from the traversal it needs to do anyway. This is roughly a wash on instructions on its own (a couple regressions, a few improvements rust-lang/rust#125910 (comment)), with a bunch of size improvements. So I'd like to land it as its own PR, then do follow-ups to take more advantage of it (in the inliner, cg_ssa, etc). r? `@saethlin`

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. labels Jun 3, 2024

scottmcm added the A-mir-opt Area: MIR optimizations label Jun 3, 2024

scottmcm force-pushed the single-use-consts branch from a02993b to fea5299 Compare June 3, 2024 04:10

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 Jun 3, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024

Auto merge of rust-lang#125910 - scottmcm:single-use-consts, r=<try>

d1611ef

Add `SingleUseConsts` mir-opt pass r? ghost

scottmcm commented Jun 3, 2024

View reviewed changes

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 Jun 3, 2024

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 Jun 3, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024

Auto merge of rust-lang#125910 - scottmcm:single-use-consts, r=<try>

620bf99

Add `SingleUseConsts` mir-opt pass r? ghost

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 Jun 3, 2024

rustbot assigned saethlin Jun 3, 2024

scottmcm marked this pull request as ready for review June 3, 2024 16:24

saethlin reviewed Jun 10, 2024

View reviewed changes

scottmcm force-pushed the single-use-consts branch from d543a8e to 82e331d Compare June 10, 2024 06:47

This comment has been minimized.

Sign in to view

scottmcm added 3 commits June 10, 2024 00:06

Add SingleUseConsts mir-opt pass

a4d0fc3

Try keeping a bitset for which locals need debuginfo updates

476f46a

Delete ConstDebugInfo pass

8fbab18

scottmcm force-pushed the single-use-consts branch from 82e331d to 8fbab18 Compare June 10, 2024 07:06

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2024

bors merged commit fa1681c into rust-lang:master Jun 11, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jun 11, 2024

bors mentioned this pull request Jun 11, 2024

Only preserve DebugInfo in DeadStoreElimination if requested. #106852

Open

scottmcm deleted the single-use-consts branch June 11, 2024 04:23

bors mentioned this pull request Jun 11, 2024

rustfmt use declarations #125443

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 11, 2024

celinval mentioned this pull request Jun 17, 2024

Upgrade Rust toolchain to nightly-2024-06-17 model-checking/kani#3271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SingleUseConsts` mir-opt pass #125910

Add `SingleUseConsts` mir-opt pass #125910

scottmcm commented Jun 3, 2024 •

edited

Loading

scottmcm commented Jun 3, 2024

This comment has been minimized.

bors commented Jun 3, 2024

scottmcm Jun 3, 2024

bors commented Jun 3, 2024

This comment has been minimized.

rust-timer commented Jun 3, 2024

scottmcm commented Jun 3, 2024

This comment has been minimized.

bors commented Jun 3, 2024

bors commented Jun 3, 2024

This comment has been minimized.

rust-timer commented Jun 3, 2024

rustbot commented Jun 3, 2024

saethlin Jun 10, 2024

scottmcm Jun 10, 2024

saethlin Jun 10, 2024 •

edited

Loading

saethlin Jun 10, 2024

scottmcm Jun 10, 2024

scottmcm Jun 10, 2024

saethlin commented Jun 10, 2024

This comment has been minimized.

scottmcm commented Jun 10, 2024

saethlin commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

rust-timer commented Jun 11, 2024

Kobzol commented Jun 11, 2024

Add SingleUseConsts mir-opt pass #125910

Add SingleUseConsts mir-opt pass #125910

Conversation

scottmcm commented Jun 3, 2024 • edited Loading

scottmcm commented Jun 3, 2024

This comment has been minimized.

bors commented Jun 3, 2024

Choose a reason for hiding this comment

bors commented Jun 3, 2024

This comment has been minimized.

rust-timer commented Jun 3, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

scottmcm commented Jun 3, 2024

This comment has been minimized.

bors commented Jun 3, 2024

bors commented Jun 3, 2024

This comment has been minimized.

rust-timer commented Jun 3, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

rustbot commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Jun 10, 2024

This comment has been minimized.

scottmcm commented Jun 10, 2024

saethlin commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

bors commented Jun 11, 2024

rust-timer commented Jun 11, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Kobzol commented Jun 11, 2024

Add `SingleUseConsts` mir-opt pass #125910

Add `SingleUseConsts` mir-opt pass #125910

scottmcm commented Jun 3, 2024 •

edited

Loading

saethlin Jun 10, 2024 •

edited

Loading