Switch the BB CFG cache from postorder to RPO #112638

lqd · 2023-06-14T23:07:29Z

The BasicBlocks CFG cache is interesting:

it stores a postorder, but traversal::postorder doesn't use it
traversal::reverse_postorder does traverse the postorder cache backwards
we do more RPO traversals than postorder traversals (around 20x on the perf.rlo benchmarks IIRC) but it's not cached
a couple places here and there were manually reversing the non-cached postorder traversal

This PR switches the order of the cache, and makes a bit more use of it. This is a tiny win locally, but it's also for consistency and aesthetics.

r? @ghost

lqd · 2023-06-14T23:08:22Z

Sanity check: @bors try @rust-timer queue

bors · 2023-06-14T23:08:32Z

⌛ Trying commit f134101 with merge 7f0a9c6034d4ceaeed051ae99e90c2d96d1a2c5e...

lqd · 2023-06-14T23:11:56Z

compiler/rustc_mir_transform/src/const_prop.rs

+        let rpo = body.basic_blocks.reverse_postorder().to_vec();
+        for bb in rpo {


@tmiasko This is where it could have been cool to avoid this existing allocation, as it's not easy to combine lazy RPO + as_mut_preserves_cfg. Probably not worth it just for this single call-site though, I don't remember seeing this elsewhere.

bors · 2023-06-15T00:40:26Z

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

bors · 2023-06-15T00:40:26Z

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

rust-timer · 2023-06-15T01:56:06Z

Finished benchmarking commit (7f0a9c6034d4ceaeed051ae99e90c2d96d1a2c5e): comparison URL.

Overall result: ❌ regressions - 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.3%, 0.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	3

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)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-1.9%	[-3.7%, -0.1%]	2
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-1.9%	[-3.7%, -0.1%]	2

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

Binary size

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

Bootstrap: 649.904s -> 650.513s (0.09%)

lqd · 2023-06-15T02:00:11Z

bitmaps looks currently bimodal on these 0.3% swings

Noratrieb · 2023-06-15T05:43:32Z

also see #112288

lqd · 2023-06-15T12:59:33Z

@Nilstrieb incredible timing :)

To me it looks like there's so few non-cached postorder traversals, that using the cache for them is a wash (all the uses of the postorder cache are the RPO traversals). Now, if we cache anything, at the very least let's cache the ordering we actually use: RPO. (But I saw this when looking to do RPO on the reverse CFG...)

That's a small but noticeable icount win in cg, and I think a cleanup. What do you think ?

Making traversal::postorder use that cache is a tiny (positive) difference in icounts as well, and I added it here for consistency, so that both traversal:: are cached, but it doesn't make a real difference on our benchmakrs, as you also saw yourself (and landing that via your PR would have made sense imho).

cjgillot

This is a nice cleanup. We should definitely get this merged.

compiler/rustc_middle/src/mir/traversal.rs

rustbot · 2023-06-18T09:23:30Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

lqd · 2023-06-18T09:23:36Z

Probably no need for another perf run.

r? @cjgillot

cjgillot · 2023-06-18T12:27:16Z

@bors r+

bors · 2023-06-18T12:27:18Z

📌 Commit 08a9f25 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-06-18T12:45:45Z

⌛ Testing commit 08a9f25 with merge 677710e...

bors · 2023-06-18T15:25:42Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 677710e to master...

rust-timer · 2023-06-18T16:41:27Z

Finished benchmarking commit (677710e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

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

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: 656.258s -> 656.279s (0.00%)

lqd added 7 commits June 14, 2023 20:08

inline explicit rpo access in promote consts

e596579

add RPO to BB CFG cache

066d381

make const-prop use cached RPO

4215859

make reorder BB pass use cached RPO

8f6e651

make traversal::reverse_postorder use RPO cache

0eec39b

make traversal::postorder traverse RPO cache backwards

0b4b086

remove unused postorder CFG cache

f134101

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 14, 2023

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 14, 2023

lqd commented Jun 14, 2023

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 15, 2023

cjgillot reviewed Jun 16, 2023

View reviewed changes

compiler/rustc_middle/src/mir/traversal.rs Outdated Show resolved Hide resolved

remove redundant combinators between PO and RPO

08a9f25

lqd marked this pull request as ready for review June 18, 2023 09:23

rustbot assigned cjgillot Jun 18, 2023

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 18, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 18, 2023

bors merged commit 677710e into rust-lang:master Jun 18, 2023

rustbot added this to the 1.72.0 milestone Jun 18, 2023

rustbot removed the perf-regression Performance regression. label Jun 18, 2023

lqd deleted the rpo branch June 18, 2023 17:55

celinval mentioned this pull request Jun 29, 2023

Upgrade rust toolchain to nightly-2023-06-20 model-checking/kani#2551

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch the BB CFG cache from postorder to RPO #112638

Switch the BB CFG cache from postorder to RPO #112638

lqd commented Jun 14, 2023

lqd commented Jun 14, 2023

This comment has been minimized.

bors commented Jun 14, 2023

lqd Jun 14, 2023

bors commented Jun 15, 2023

bors commented Jun 15, 2023

This comment has been minimized.

rust-timer commented Jun 15, 2023

lqd commented Jun 15, 2023

Noratrieb commented Jun 15, 2023

lqd commented Jun 15, 2023 •

edited

Loading

cjgillot left a comment

rustbot commented Jun 18, 2023

lqd commented Jun 18, 2023

cjgillot commented Jun 18, 2023

bors commented Jun 18, 2023

bors commented Jun 18, 2023

bors commented Jun 18, 2023

rust-timer commented Jun 18, 2023

		let rpo = body.basic_blocks.reverse_postorder().to_vec();
		for bb in rpo {

Switch the BB CFG cache from postorder to RPO #112638

Switch the BB CFG cache from postorder to RPO #112638

Conversation

lqd commented Jun 14, 2023

lqd commented Jun 14, 2023

This comment has been minimized.

bors commented Jun 14, 2023

Choose a reason for hiding this comment

bors commented Jun 15, 2023

bors commented Jun 15, 2023

This comment has been minimized.

rust-timer commented Jun 15, 2023

Overall result: ❌ regressions - ACTION NEEDED

lqd commented Jun 15, 2023

Noratrieb commented Jun 15, 2023

lqd commented Jun 15, 2023 • edited Loading

cjgillot left a comment

Choose a reason for hiding this comment

rustbot commented Jun 18, 2023

lqd commented Jun 18, 2023

cjgillot commented Jun 18, 2023

bors commented Jun 18, 2023

bors commented Jun 18, 2023

bors commented Jun 18, 2023

rust-timer commented Jun 18, 2023

Overall result: no relevant changes - no action needed

lqd commented Jun 15, 2023 •

edited

Loading