Add a debug-info cost to MIR inlining #123011

scottmcm · 2024-03-24T18:32:04Z

Inspired by all the MIR I keep looking at that's inlined away like 7 different function calls so the body is 2 statements but 30 debug-infos.

r? ghost

scottmcm · 2024-03-24T18:32:15Z

@bors try @rust-timer queue

[EXPERIMENT] Add a debug-info cost to MIR inlining r? ghost

bors · 2024-03-24T18:33:24Z

⌛ Trying commit 21e0519 with merge 94c5411...

workingjubilee · 2024-03-24T18:42:40Z

@scottmcm just curious, is there a particular problem with having 100 debuginfos and 1 statement?

scottmcm · 2024-03-24T18:49:40Z

@workingjubilee Not exactly a problem per se, just that I think we shouldn't treat that as basically-free for inlining. Copying them all into every caller just because it's cheap at runtime still has impact on memory usage and rlib sizes and such.

I expect this to actually change the inlining decision quite rarely -- the cost used given to them is extremely small. But since it's also really cheap to compute this cost, I'm guessing it's still worth it to have some limit on how much stuff we'd inline.

bors · 2024-03-24T20:11:05Z

☀️ Try build successful - checks-actions
Build commit: 94c5411 (94c5411900a8579b1ab401bd9208a9374d05a3e0)

rust-timer · 2024-03-24T23:10:16Z

Finished benchmarking commit (94c5411): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

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)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.4%	[-5.8%, -1.8%]	3
All ❌✅ (primary)	2.6%	[2.6%, 2.6%]	1

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

Binary size

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

Bootstrap: 670.98s -> 670.188s (-0.12%)
Artifact size: 315.04 MiB -> 315.05 MiB (0.00%)

scottmcm · 2024-03-25T01:45:38Z

@bors try @rust-timer queue

bors · 2024-03-25T01:46:48Z

⌛ Trying commit 9bae087 with merge ea37c63...

[EXPERIMENT] Add a debug-info cost to MIR inlining Inspired by all the MIR I keep looking at that's inlined away like 7 different function calls so the body is 2 statements but 30 debug-infos. r? ghost

bors · 2024-03-25T03:18:04Z

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

rust-timer · 2024-03-25T04:31:40Z

Finished benchmarking commit (ea37c63): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

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

Cycles

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

Binary size

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

Bootstrap: 670.209s -> 670.51s (0.04%)
Artifact size: 315.07 MiB -> 315.05 MiB (-0.01%)

scottmcm · 2024-03-25T06:19:24Z

Well, those results look great, but the two big wins in wall time look like they're noise.

I still think this is worth doing, since it's not regressing anything and should help the outliers. But let's see what a reviewer has to say...
r? mir-opt
@rustbot ready

rustbot · 2024-03-25T06:19:30Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

wesleywiser

This seems fine to me as MIR inlining cost (imperfectly) represents LLVM IR size which debuginfo certainly contributes to.

wesleywiser · 2024-03-25T23:35:38Z

@bors r+

Add a debug-info cost to MIR inlining Inspired by all the MIR I keep looking at that's inlined away like 7 different function calls so the body is 2 statements but 30 debug-infos. r? ghost

bors · 2024-03-26T21:13:41Z

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

rust-timer · 2024-03-26T22:52:41Z

Finished benchmarking commit (a40a62a): 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.5%	[0.2%, 1.4%]	27
Regressions ❌ (secondary)	0.6%	[0.2%, 2.1%]	21
Improvements ✅ (primary)	-0.6%	[-1.3%, -0.2%]	15
Improvements ✅ (secondary)	-1.1%	[-3.4%, -0.3%]	9
All ❌✅ (primary)	0.1%	[-1.3%, 1.4%]	42

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.9%	[1.9%, 1.9%]	1
Regressions ❌ (secondary)	4.8%	[4.8%, 4.8%]	1
Improvements ✅ (primary)	-2.3%	[-6.7%, -0.2%]	21
Improvements ✅ (secondary)	-3.5%	[-4.7%, -2.4%]	7
All ❌✅ (primary)	-2.1%	[-6.7%, 1.9%]	22

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)	1.0%	[0.7%, 1.6%]	4
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-0.8%	[-1.1%, -0.7%]	7
Improvements ✅ (secondary)	-3.5%	[-4.3%, -3.0%]	3
All ❌✅ (primary)	-0.1%	[-1.1%, 1.6%]	11

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.3%]	28
Regressions ❌ (secondary)	2.8%	[0.5%, 14.7%]	9
Improvements ✅ (primary)	-1.0%	[-3.9%, -0.0%]	67
Improvements ✅ (secondary)	-2.2%	[-3.9%, -0.1%]	12
All ❌✅ (primary)	-0.6%	[-3.9%, 0.3%]	95

Bootstrap: 671.741s -> 673.856s (0.31%)
Artifact size: 315.68 MiB -> 315.36 MiB (-0.10%)

scottmcm · 2024-03-26T22:58:42Z

Ah, that makes more sense. Exactly the kind of messy non-obvious perf result I would have expected for an inlining change 😆

scottmcm · 2024-03-27T03:27:47Z

@bors try @rust-timer queue

bors · 2024-03-27T03:28:58Z

⌛ Trying commit 989bee0 with merge 628e4f4...

Add a debug-info cost to MIR inlining Inspired by all the MIR I keep looking at that's inlined away like 7 different function calls so the body is 2 statements but 30 debug-infos. r? ghost

bors · 2024-03-27T05:01:01Z

☀️ Try build successful - checks-actions
Build commit: 628e4f4 (628e4f4162095c0128bac5fc3c2dd4720b68fc4b)

bors · 2024-03-27T05:01:01Z

☀️ Try build successful - checks-actions
Build commit: 628e4f4 (628e4f4162095c0128bac5fc3c2dd4720b68fc4b)

rust-log-analyzer · 2024-03-27T08:09:03Z

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#17 exporting to docker image format
#17 sending tarball 27.9s done
#17 DONE 33.4s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
.......................

failures:

---- [mir-opt] tests/mir-opt/pre-codegen/checked_ops.rs stdout ----
4     debug x => _1;
5     debug n => _2;
6     let mut _0: u32;
+     scope 1 (inlined <u32 as Step>::forward) {
+         debug start => _1;
+         debug n => _2;
+         let _3: std::option::Option<u32>;
+         let mut _4: &std::option::Option<u32>;
+         let mut _6: u32;
+         scope 2 {
+         }
+         scope 3 (inlined Option::<u32>::is_none) {
+             debug self => _4;
+             scope 4 (inlined Option::<u32>::is_some) {
+                 debug self => _4;
+                 let mut _5: isize;
+         }
+         }
+         scope 5 (inlined core::num::<impl u32>::wrapping_add) {
+             debug self => _1;
+             debug rhs => _6;
+     }
7 
8     bb0: {
Build completed unsuccessfully in 0:14:07
Build completed unsuccessfully in 0:14:07
-         _0 = <u32 as Step>::forward(move _1, move _2) -> [return: bb1, unwind continue];
+         StorageLive(_4);
+         StorageLive(_3);
+         _3 = <u32 as Step>::forward_checked(_1, _2) -> [return: bb1, unwind continue];
11 
12     bb1: {

+         _4 = &_3;
+         _4 = &_3;
+         StorageLive(_5);
+         _5 = discriminant(_3);
+         switchInt(move _5) -> [1: bb2, 0: bb3, otherwise: bb5];
+ 
+     bb2: {
+         StorageDead(_5);
+         StorageDead(_3);
+         StorageDead(_3);
+         StorageDead(_4);
+         goto -> bb4;
+     }
+ 
+     bb3: {
+         StorageDead(_5);
+         StorageDead(_3);
+         StorageDead(_4);
+         assert(!const true, "attempt to compute `{} + {}`, which would overflow", const core::num::<impl u32>::MAX, const 1_u32) -> [success: bb4, unwind continue];
+ 
+     bb4: {
+         StorageLive(_6);
+         StorageLive(_6);
+         _6 = _2 as u32 (IntToInt);
+         _0 = Add(_1, _6);
+         StorageDead(_6);
+     }
+ 
+     bb5: {
+         unreachable;
+         unreachable;
14     }
15 }
16 

thread '[mir-opt] tests/mir-opt/pre-codegen/checked_ops.rs' panicked at src/tools/compiletest/src/runtest.rs:4351:21:
Actual MIR output differs from expected MIR output /checkout/tests/mir-opt/pre-codegen/checked_ops.step_forward.PreCodegen.after.mir


failures:
    [mir-opt] tests/mir-opt/pre-codegen/checked_ops.rs

rust-timer · 2024-03-27T09:16:33Z

Finished benchmarking commit (628e4f4): 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.3%	[0.2%, 0.5%]	10
Regressions ❌ (secondary)	0.6%	[0.2%, 2.0%]	18
Improvements ✅ (primary)	-0.9%	[-1.0%, -0.9%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-1.0%, 0.5%]	12

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.8%	[1.3%, 6.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-6.2%, -0.2%]	18
Improvements ✅ (secondary)	-3.7%	[-7.4%, -1.8%]	46
All ❌✅ (primary)	-1.5%	[-6.2%, 6.2%]	20

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.5%, 0.5%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	2
Improvements ✅ (primary)	-0.9%	[-1.1%, -0.6%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-1.1%, 0.5%]	5

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.1%, 0.6%]	14
Regressions ❌ (secondary)	5.2%	[0.4%, 14.8%]	3
Improvements ✅ (primary)	-0.4%	[-1.8%, -0.0%]	74
Improvements ✅ (secondary)	-0.2%	[-1.5%, -0.0%]	53
All ❌✅ (primary)	-0.3%	[-1.8%, 0.6%]	88

Bootstrap: 670.407s -> 672.371s (0.29%)
Artifact size: 315.71 MiB -> 315.45 MiB (-0.08%)

mir-inline: use the visitor to walk blocks Today it's manually DFS-traversing them; let's see if that's actually fruitful. Inspired by <rust-lang#123011 (comment)> & <https://discord.com/channels/273534239310479360/957720175619215380/1222325151731945503> r? ghost

scottmcm · 2024-03-29T03:19:49Z

Closing in favour of #123179

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 Mar 24, 2024