Split phase change from `MirPass` #102340

JakobDegen · 2022-09-27T01:49:45Z

The main goal here is to simplify the pass manager logic. MirPass no longer contains the phase_change method, and run_passes instead accepts an Option<PhaseChange>. The hope is that this addresses the comments (and maybe perf regression) from #99102 .

r? @oli-obk cc @RalfJung

rustbot · 2022-09-27T01:49:48Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

JakobDegen · 2022-09-27T10:35:30Z

@bors try @rust-timer queue

rust-timer · 2022-09-27T10:35:32Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-09-27T10:35:44Z

⌛ Trying commit b6bd3d8deae8ab0c97efacc73a53a932608d1c78 with merge 91f1e60cacbc612e315bf021ed4cf3dbd485dfcc...

bors · 2022-09-27T12:33:07Z

☀️ Try build successful - checks-actions
Build commit: 91f1e60cacbc612e315bf021ed4cf3dbd485dfcc (91f1e60cacbc612e315bf021ed4cf3dbd485dfcc)

rust-timer · 2022-09-27T12:33:09Z

Queued 91f1e60cacbc612e315bf021ed4cf3dbd485dfcc with parent de0b511, future comparison URL.

rust-timer · 2022-09-27T15:53:06Z

Finished benchmarking commit (91f1e60cacbc612e315bf021ed4cf3dbd485dfcc): comparison URL.

Overall result: ✅ improvements - 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-review -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.3%]	4
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	1

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)	-	-	0
Improvements ✅ (secondary)	-5.0%	[-6.8%, -3.1%]	2
All ❌✅ (primary)	-	-	0

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.4%	[-5.3%, -3.3%]	3
All ❌✅ (primary)	-	-	0

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

RalfJung · 2022-09-27T15:58:23Z

compiler/rustc_middle/src/mir/mod.rs

@@ -292,6 +319,7 @@ impl<'tcx> Body<'tcx> {

        let mut body = Body {
            phase: MirPhase::Built,
+            pass_count: 1,


That looks like pretty fragile initialization. Is this always 1 initially?

Yeah, this is a counter that is always set/reset to 1 everywhere.

JakobDegen · 2022-09-28T01:03:24Z

The revert is entirely a shot in the dark for getting the perf from #99102 back, but let's see (waiting on CI before queuing)

JakobDegen · 2022-09-28T04:29:12Z

@bors try @rust-timer queue

rust-timer · 2022-09-28T04:29:13Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-09-28T04:29:21Z

⌛ Trying commit 43ea3c1d6b34a9d562c2d72e3497c1c1e73ef65e with merge 43ae51f3bc0705a23cc4c354a410097b33a2ef65...

bors · 2022-09-28T05:51:44Z

☀️ Try build successful - checks-actions
Build commit: 43ae51f3bc0705a23cc4c354a410097b33a2ef65 (43ae51f3bc0705a23cc4c354a410097b33a2ef65)

rust-timer · 2022-09-28T05:51:46Z

Queued 43ae51f3bc0705a23cc4c354a410097b33a2ef65 with parent d6734be, future comparison URL.

rust-timer · 2022-09-28T07:20:50Z

Finished benchmarking commit (43ae51f3bc0705a23cc4c354a410097b33a2ef65): 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-review -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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	1.3%	[1.2%, 1.5%]	6
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	3
All ❌✅ (primary)	0.2%	[-0.2%, 0.7%]	2

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

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)	8.1%	[8.1%, 8.1%]	1
Regressions ❌ (secondary)	5.9%	[5.9%, 5.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.2%, -2.9%]	2
All ❌✅ (primary)	8.1%	[8.1%, 8.1%]	1

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

JakobDegen · 2022-10-23T21:39:49Z

I've dropped the last commit (which turned out to not give back any perf benefits). I'll keep looking into fixing the perf regression from the other PR, but in the meantime I think we should probably merge this, since it's an improvement on its own.

oli-obk · 2022-10-24T19:29:04Z

@bors r+

bors · 2022-10-24T19:29:06Z

📌 Commit be2401b has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-10-24T20:48:51Z

⌛ Testing commit be2401b with merge 051edf415cb5a12984d6976d7df4f50ed60e0bea...

bors · 2022-10-25T01:03:52Z

💥 Test timed out

rust-log-analyzer · 2022-10-25T02:28:02Z

A job failed! Check out the build log: (web) (plain)

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

JakobDegen · 2022-10-25T03:36:26Z

Was the failure spurious? How do I tell?

oli-obk · 2022-10-25T06:04:39Z

@bors retry timeout

Probably just an arbitrary spurious CI failure

bors · 2022-10-25T15:55:42Z

⌛ Testing commit be2401b with merge bed4ad6...

bors · 2022-10-25T19:03:49Z

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

rust-timer · 2022-10-25T20:21:38Z

Finished benchmarking commit (bed4ad6): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-1.3%, -0.2%]	4
All ❌✅ (primary)	-	-	0

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)	9.9%	[2.3%, 20.0%]	22
Regressions ❌ (secondary)	3.5%	[3.2%, 3.8%]	2
Improvements ✅ (primary)	-6.1%	[-6.6%, -5.2%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	7.5%	[-6.6%, 20.0%]	26

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)	10.5%	[0.9%, 41.9%]	95
Regressions ❌ (secondary)	6.1%	[1.2%, 15.0%]	36
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-3.2%, -3.2%]	1
All ❌✅ (primary)	10.5%	[0.9%, 41.9%]	95

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

Mark-Simulacrum · 2022-10-25T23:06:58Z

FYI using this commit to get a SMT + Turbo vs. non-SMT and non-Turbo comparison on the perf machine, expecting another post here as a result. See discussion here https://rust-lang.zulipchat.com/#narrow/stream/247081-t-compiler.2Fperformance/topic/Disabling.20hyperthreading.2Fturbo/near/306100379

rust-timer · 2022-10-26T00:22:20Z

Finished benchmarking commit (bed4ad6): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-1.3%, -0.2%]	4
All ❌✅ (primary)	-	-	0

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

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)	-	-	0
Improvements ✅ (primary)	-2.4%	[-3.8%, -1.4%]	37
Improvements ✅ (secondary)	-2.8%	[-3.4%, -2.1%]	17
All ❌✅ (primary)	-2.4%	[-3.8%, -1.4%]	37

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

…on, r=oli-obk Split phase change from `MirPass` The main goal here is to simplify the pass manager logic. `MirPass` no longer contains the `phase_change` method, and `run_passes` instead accepts an `Option<PhaseChange>`. The hope is that this addresses the comments (and maybe perf regression) from rust-lang#99102 . r? `@oli-obk` cc `@RalfJung`

rust-highfive assigned oli-obk Sep 27, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2022

JakobDegen mentioned this pull request Sep 27, 2022

Add mir building test directory #101428

Merged

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 27, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 27, 2022

RalfJung reviewed Sep 27, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 28, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 28, 2022

Split phase change from MirPass

be2401b

JakobDegen force-pushed the pass-manager-simplification branch from 43ea3c1 to be2401b Compare October 23, 2022 21:19

JakobDegen mentioned this pull request Oct 24, 2022

Add support for custom mir #103464

Merged

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 Oct 24, 2022

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 Oct 25, 2022

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 Oct 25, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 25, 2022

bors merged commit bed4ad6 into rust-lang:master Oct 25, 2022

rustbot added this to the 1.66.0 milestone Oct 25, 2022

This was referenced Oct 25, 2022

Separate generator info from MIR body. #101547

Closed

Compute generator saved locals on MIR #101692

Merged

rustbot removed the perf-regression Performance regression. label Oct 25, 2022

JakobDegen deleted the pass-manager-simplification branch October 27, 2022 00:00

compiler-errors mentioned this pull request Jun 6, 2023

inlining produces broken MIR by changing whether types can be normalized #112332

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split phase change from `MirPass` #102340

Split phase change from `MirPass` #102340

JakobDegen commented Sep 27, 2022

rustbot commented Sep 27, 2022

JakobDegen commented Sep 27, 2022

rust-timer commented Sep 27, 2022

bors commented Sep 27, 2022

bors commented Sep 27, 2022

rust-timer commented Sep 27, 2022

rust-timer commented Sep 27, 2022

RalfJung Sep 27, 2022

JakobDegen Sep 27, 2022

JakobDegen commented Sep 28, 2022

JakobDegen commented Sep 28, 2022

rust-timer commented Sep 28, 2022

bors commented Sep 28, 2022

bors commented Sep 28, 2022

rust-timer commented Sep 28, 2022

rust-timer commented Sep 28, 2022

JakobDegen commented Oct 23, 2022

oli-obk commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 25, 2022

rust-log-analyzer commented Oct 25, 2022

JakobDegen commented Oct 25, 2022

oli-obk commented Oct 25, 2022

bors commented Oct 25, 2022

bors commented Oct 25, 2022

rust-timer commented Oct 25, 2022

Mark-Simulacrum commented Oct 25, 2022

rust-timer commented Oct 26, 2022

Split phase change from MirPass #102340

Split phase change from MirPass #102340

Conversation

JakobDegen commented Sep 27, 2022

rustbot commented Sep 27, 2022

JakobDegen commented Sep 27, 2022

rust-timer commented Sep 27, 2022

bors commented Sep 27, 2022

bors commented Sep 27, 2022

rust-timer commented Sep 27, 2022

rust-timer commented Sep 27, 2022

Overall result: ✅ improvements - no action needed

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobDegen commented Sep 28, 2022

JakobDegen commented Sep 28, 2022

rust-timer commented Sep 28, 2022

bors commented Sep 28, 2022

bors commented Sep 28, 2022

rust-timer commented Sep 28, 2022

rust-timer commented Sep 28, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

JakobDegen commented Oct 23, 2022

oli-obk commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 25, 2022

rust-log-analyzer commented Oct 25, 2022

JakobDegen commented Oct 25, 2022

oli-obk commented Oct 25, 2022

bors commented Oct 25, 2022

bors commented Oct 25, 2022

rust-timer commented Oct 25, 2022

Overall result: ❌✅ regressions and improvements - no action needed

Footnotes

Mark-Simulacrum commented Oct 25, 2022

rust-timer commented Oct 26, 2022

Overall result: ✅ improvements - no action needed

Footnotes

Split phase change from `MirPass` #102340

Split phase change from `MirPass` #102340