Accelerate GVN a little #126991

cjgillot · 2024-06-26T14:07:15Z

This PR addresses a few inefficiencies I've seen in callgrind profiles.

Commits are independent.

Only the first commit introduces a change in behaviour: we stop substituting some constant pointers. But we keep propagating their contents that have no provenance, so we don't lose much.

r? @saethlin

rustbot · 2024-06-26T14:07:23Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

cjgillot · 2024-06-26T14:09:04Z

@bors try @rust-timer queue

Accelerate GVN a little This PR addresses a few inefficiencies I've seen in callgrind profiles. Commits are independent. Only the first commit introduces a change in behaviour: we stop substituting some constant pointers. But we keep propagating their contents that have no provenance, so we don't lose much. r? `@saethlin`

bors · 2024-06-26T14:10:16Z

⌛ Trying commit c633384 with merge dfaffed...

bors · 2024-06-26T15:50:10Z

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

rust-timer · 2024-06-26T17:05:18Z

Finished benchmarking commit (dfaffed): 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.4%, 0.8%]	3
Regressions ❌ (secondary)	2.4%	[0.2%, 4.5%]	2
Improvements ✅ (primary)	-0.4%	[-0.7%, -0.2%]	3
Improvements ✅ (secondary)	-0.5%	[-0.8%, -0.2%]	7
All ❌✅ (primary)	0.1%	[-0.7%, 0.8%]	6

Max RSS (memory usage)

Results (primary -0.6%, secondary 2.0%)

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.1%	[2.9%, 9.3%]	2
Regressions ❌ (secondary)	4.9%	[3.3%, 5.9%]	7
Improvements ✅ (primary)	-4.0%	[-7.0%, -2.0%]	4
Improvements ✅ (secondary)	-4.8%	[-7.7%, -3.4%]	3
All ❌✅ (primary)	-0.6%	[-7.0%, 9.3%]	6

Cycles

Results (primary 2.4%, secondary 2.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.4%	[2.0%, 2.7%]	4
Regressions ❌ (secondary)	3.0%	[2.6%, 3.5%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-4.6%, -2.8%]	2
All ❌✅ (primary)	2.4%	[2.0%, 2.7%]	4

Binary size

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

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.0%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.6%, -0.0%]	55
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.4%]	3
All ❌✅ (primary)	-0.1%	[-0.6%, 0.0%]	59

Bootstrap: 695.46s -> 692.615s (-0.41%)
Artifact size: 326.69 MiB -> 326.64 MiB (-0.01%)

scottmcm · 2024-06-26T23:44:35Z

compiler/rustc_mir_transform/src/gvn.rs

@@ -264,21 +265,29 @@ struct VnState<'body, 'tcx> {
 impl<'body, 'tcx> VnState<'body, 'tcx> {
    fn new(
        tcx: TyCtxt<'tcx>,
+        body: &Body<'tcx>,
        param_env: ty::ParamEnv<'tcx>,
        ssa: &'body SsaLocals,
        dominators: &'body Dominators<BasicBlock>,
        local_decls: &'body LocalDecls<'tcx>,


nit: I think you could get this from the passed-in body now, rather than needing the parameter?

No, they have different lifetimes. The caller will want to mutate part of the body, so we can only keep a reference to the local decls.

saethlin · 2024-06-26T23:54:57Z

Hm. I think the behavior change here is more significant to compile time than any of the optimizations. Is that expected?

I ran this to build the standard library and dump MIR with each of the toolchains in the perf run

RUSTFLAGS="--emit=mir -Zmir-enable-passes=+ReorderBasicBlocks,+ReorderLocals" cargo +dfaffedda67fd2f26a394763df8cda0a5e58d672 b --release -Zbuild-std --target=x86_64-unknown-linux-gnu

Then did a git diff --no-index on the old core MIR and the new one and I get:

 .../x86_64-unknown-linux-gnu/release/deps/core-5d5f6913076bfe42.mir              | 17178 ++++++++++++-------------------------
 1 file changed, 5525 insertions(+), 11653 deletions(-)

Almost all the net deletion is due to the MIR dumps having less const allocations. That's surprising to me. I can tell from the commits that this is caused by normalizing less... but why does this even matter? If these consts can just be deleted from the MIR, shouldn't they have been deduplicated in the first place by some other mechanism?

cjgillot · 2024-06-27T08:30:10Z

tests/mir-opt/const_prop/slice_len.main.GVN.32bit.panic-abort.diff

-+         _3 = const {ALLOC0<imm>: &[u32; 3]};
-+         _2 = const {ALLOC0<imm>: &[u32; 3]} as &[u32] (PointerCoercion(Unsize));
+         _3 = _9;
+         _2 = _9 as &[u32] (PointerCoercion(Unsize));


@saethlin this is this kind of pattern.
The constant is still there, named main::promoted[0]. It's "evaluated" form const {ALLOC0<imm>: &[u32; 3]} does not appear in the MIR dump, so the dump does not print ALLOC0. This does not mean that ALLOC0 was not evaluated, just that it's not put in MIR and not printed.

So are you saying there consts that get their allocations dumped, and some consts that don't? That seems like an undesirable property of MIR dumps, right?

yea, Ideally promoteds would just be regular statics, but lazily created in the MIR opt pipeline. This is blocked on various works around DefId creation, most importantly #115613. I guess we could add a special case for showing promoteds like we show other allocations for now, but that seems orthogonal to this PR

Statics can't be generic so IMO they should be regular consts.

cjgillot · 2024-06-27T09:14:56Z

About perf:

the main regression is for deep-vector, on a check build, so I'd classify this as noise;
for other regressions (webrender, ripgrep), detailed results show GVN taking less time, and LLVM taking more.

I'm very much tempted to leave this as-is.

oli-obk · 2024-07-25T08:53:36Z

@bors r+

bors · 2024-07-25T08:53:39Z

📌 Commit 8279989 has been approved by oli-obk

It is now in the queue for this repository.

Accelerate GVN a little This PR addresses a few inefficiencies I've seen in callgrind profiles. Commits are independent. Only the first commit introduces a change in behaviour: we stop substituting some constant pointers. But we keep propagating their contents that have no provenance, so we don't lose much. r? `@saethlin`

bors · 2024-07-25T10:17:21Z

⌛ Testing commit 8279989 with merge 4f32f47...

bors · 2024-07-25T10:37:27Z

💔 Test failed - checks-actions

oli-obk · 2024-07-31T14:58:13Z

@bors r+

bors · 2024-07-31T14:58:15Z

📌 Commit 4067795 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-07-31T18:37:23Z

⌛ Testing commit 4067795 with merge 28a58f2...

bors · 2024-07-31T20:59:35Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 28a58f2 to master...

rust-timer · 2024-08-01T20:25:19Z

Finished benchmarking commit (28a58f2): 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.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.3%]	2
Improvements ✅ (secondary)	-0.7%	[-1.0%, -0.4%]	6
All ❌✅ (primary)	-0.2%	[-0.4%, 0.3%]	3

Max RSS (memory usage)

Results (primary 4.1%, secondary 1.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)	4.1%	[3.2%, 5.0%]	2
Regressions ❌ (secondary)	4.1%	[3.4%, 4.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-3.8%, -3.8%]	1
All ❌✅ (primary)	4.1%	[3.2%, 5.0%]	2

Cycles

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

Binary size

Results (primary -0.1%, 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.0%	[0.0%, 0.1%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.0%]	56
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.4%]	3
All ❌✅ (primary)	-0.1%	[-0.6%, 0.1%]	59

Bootstrap: 755.782s -> 757.595s (0.24%)
Artifact size: 336.64 MiB -> 336.69 MiB (0.01%)

nnethercote · 2024-08-01T21:00:06Z

Nice binary size reductions :)

Kobzol · 2024-08-06T13:18:23Z

More improvements than regressions, and also nice binary size regressions, as Nick said.

@rustbot label: +perf-regression-triaged

rustbot assigned saethlin Jun 26, 2024

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 26, 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 26, 2024

This comment has been minimized.

Sign in to view

oli-obk self-assigned this Jun 26, 2024

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 26, 2024

cjgillot force-pushed the gvn-prof branch from c633384 to 8279989 Compare June 26, 2024 17:32

scottmcm reviewed Jun 26, 2024

View reviewed changes

cjgillot commented Jun 27, 2024

View reviewed changes

cjgillot added perf-regression-triaged The performance regression has been triaged. A-mir-opt Area: MIR optimizations labels Jun 27, 2024

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 Jul 25, 2024

This comment has been minimized.

Sign in to view

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

cjgillot added 7 commits July 31, 2024 00:59

Do not normalize constants eagerly.

a0b4d6d

Indirect places can only appear as first projection in runtime MIR.

95986dd

Amortize growing rev_locals.

70ee6e4

Reduce allocations in GVN.

9d23c86

Encode constant determinism in disambiguator.

61ef044

Simplify constant creation.

98c1ea8

Do not intern if we have provenance.

4067795

cjgillot force-pushed the gvn-prof branch from 8279989 to 4067795 Compare July 31, 2024 00:59

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 Jul 31, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 31, 2024

bors merged commit 28a58f2 into rust-lang:master Jul 31, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Jul 31, 2024

cjgillot deleted the gvn-prof branch August 1, 2024 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accelerate GVN a little #126991

Accelerate GVN a little #126991

cjgillot commented Jun 26, 2024

rustbot commented Jun 26, 2024

cjgillot commented Jun 26, 2024

This comment has been minimized.

bors commented Jun 26, 2024

This comment has been minimized.

bors commented Jun 26, 2024

This comment has been minimized.

rust-timer commented Jun 26, 2024

scottmcm Jun 26, 2024

cjgillot Jun 27, 2024

saethlin commented Jun 26, 2024

cjgillot Jun 27, 2024

saethlin Jun 27, 2024

oli-obk Jul 25, 2024

RalfJung Aug 9, 2024

cjgillot commented Jun 27, 2024

oli-obk commented Jul 25, 2024

bors commented Jul 25, 2024

bors commented Jul 25, 2024

This comment has been minimized.

bors commented Jul 25, 2024

oli-obk commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Jul 31, 2024

rust-timer commented Aug 1, 2024

nnethercote commented Aug 1, 2024

Kobzol commented Aug 6, 2024

Accelerate GVN a little #126991

Accelerate GVN a little #126991

Conversation

cjgillot commented Jun 26, 2024

rustbot commented Jun 26, 2024

cjgillot commented Jun 26, 2024

This comment has been minimized.

bors commented Jun 26, 2024

This comment has been minimized.

bors commented Jun 26, 2024

This comment has been minimized.

rust-timer commented Jun 26, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Jun 26, 2024

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

cjgillot commented Jun 27, 2024

oli-obk commented Jul 25, 2024

bors commented Jul 25, 2024

bors commented Jul 25, 2024

This comment has been minimized.

bors commented Jul 25, 2024

oli-obk commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Jul 31, 2024

bors commented Jul 31, 2024

rust-timer commented Aug 1, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote commented Aug 1, 2024

Kobzol commented Aug 6, 2024