Separate generator info from MIR body.

[cjgillot]

Split from #99782.

cc @JakobDegen does the splitting make sense with your recent refactor of MIR phases.
cc @eholk this should make the late generator analysis easier to work with.

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

[rust-highfive]

r? @compiler-errors

(rust-highfive has picked a reviewer for you, use r? to override)

[eholk]

I did a quick read over this and it looks good to me! I left a few comments with some questions just to make sure I'm understanding things right.

[JakobDegen]

cc @JakobDegen does the splitting make sense with your recent refactor of MIR phases.

See the documentation on MirPhase; the generator transform is identified there as a semantic change (because locals before a generator transform aren't really locals). This means it needs to remain part of a dialect transition, and cannot just be a phase.

Do we even still need mir_drops_elaborated_and_consts_checked? Can't we just replace it with mir_generators_lowered?

[JakobDegen]

Also note that this PR removes a couple stale files from the mir-opt tests. That was my fault, it's fixed on master though, so should be fine if you rebase.

[RalfJung]

This needs a doc comment (that's pre-existing though, so probably material for a different PR).

[cjgillot]

See the documentation on MirPhase; the generator transform is identified there as a semantic change (because locals before a generator transform aren't really locals). This means it needs to remain part of a dialect transition, and cannot just be a phase.

I don't understand your comment. Does this mean it has to change the variant of MirPhase and not only of RuntimePhase?

Do we even still need mir_drops_elaborated_and_consts_checked? Can't we just replace it with mir_generators_lowered?

I've put back the transformation to its original position. This avoids introducing an additional query.

[cjgillot]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 04c480cf368292bf263ab75600d90e73dada2751 with merge d3f203dbddcb1d3835cbc318a73f4725d8dd540d...

[bors]

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

[rust-timer]

Queued d3f203dbddcb1d3835cbc318a73f4725d8dd540d with parent 8778809, future comparison URL.

[rust-timer]

Finished benchmarking commit (d3f203dbddcb1d3835cbc318a73f4725d8dd540d): 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-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.

	mean1	range	count2
Regressions ❌ (primary)	0.4%	[0.2%, 0.9%]	42
Regressions ❌ (secondary)	0.7%	[0.4%, 1.2%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.9%]	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.

	mean1	range	count2
Regressions ❌ (primary)	1.6%	[1.2%, 2.1%]	3
Regressions ❌ (secondary)	2.1%	[1.4%, 2.9%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-2.7%, -0.6%]	2
All ❌✅ (primary)	1.6%	[1.2%, 2.1%]	3

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[bors]

☔ The latest upstream changes (presumably #101086) made this pull request unmergeable. Please resolve the merge conflicts.

[compiler-errors]

@cjgillot sorry I haven't been keeping up with this PR -- not sure if I'm qualified to make the call about whether to accept the perf regression. Do others have thoughts? (@JakobDegen @eholk to name a few people already involved in this PR)

[cjgillot]

@compiler-errors this PR only really makes sense in the context of #99782 or #101692. Without those PRs, there is no reason to pay the perf cost of this one. I'd suggest we stall this PR on a decision on those.

[bors]

☔ The latest upstream changes (presumably #103172) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #102340) made this pull request unmergeable. Please resolve the merge conflicts.

[apiraino]

@cjgillot checking the pulse of this PR. Based on your comment what are your thoughts now about this PR (#99782 was closed, #101692 is making progesses).

thanks for your work :)

[compiler-errors]

Marking as blocked until an update is provided about plans for this -- specifically, still waiting on #101692?

[cjgillot]

This PR waits for a decision on #101692.
I will close it, as it's just a subset of the commits.