Stop storing a special inner body for the coroutine by-move body for async closures #128506

compiler-errors · 2024-08-01T17:16:16Z

...and instead, just synthesize an item which is treated mostly normally by the MIR pipeline.

This PR does a few things:

We synthesize a new DefId for the by-move body of a closure, which has its mir_built fed with the output of the ByMoveBody MIR transformation, and some other relevant queries.
This has the ~~DefKind::ByMoveBody~~ DefKind::SyntheticCoroutineBody, which we use to distinguish it from "real" bodies (that come from HIR) which need to be borrowck'd. Introduce TyCtxt::is_synthetic_mir to skip over mir_borrowck which is called by mir_promoted; borrowck isn't really possible to make work ATM since it heavily relies being called on a body generated from HIR, and is redundant by the construction of the by-move-body.
Remove the special PassManager hacks for handling the inner by_move_body stored within the coroutine's mir body. Instead, this body is fed like a regular MIR body, so it's goes through all of the tcx.*_mir stages normally (build -> promoted -> ...etc... -> optimized) ✨.
Remove the InstanceKind::ByMoveBody shim, since now we have a "regular" def id, we can just use InstanceKind::Item. This also allows us to remove the corresponding hacks from codegen, such as in fn_sig_for_fn_abi ✨.

Notable remarks:

I know it's kind of weird to be using DefKind::Closure here, since it's not a distinct closure but just a new MIR body. I don't believe it really matters, but I could also use a different DefKind... maybe one that we could use for synthetic MIR bodies in general? edit: We're doing this now.

rustbot · 2024-08-01T17:16:25Z

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-08-01T17:16:27Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2024-08-01T17:17:25Z

Anywhomst, cc @RalfJung if you're curious about what this ended up looking like.

compiler-errors · 2024-08-01T17:21:36Z

compiler/rustc_mir_transform/src/validate.rs

@@ -102,25 +102,6 @@ impl<'tcx> MirPass<'tcx> for Validator {
                }
            }
        }
-
-        // Enforce that coroutine-closure layouts are identical.


We stopped caring about this since #123021.

compiler-errors · 2024-08-01T17:25:16Z

Let's get perf kicked off. I'll fix the codegen tests when I get home.

@bors try @rust-timer queue

Stop storing a special inner body for the coroutine by-move body for async closures ...and instead, just synthesize an item which is treated mostly normally by the MIR pipeline. This PR does a few things: * We synthesize a new `DefId` for the by-move body of a closure, which has its `mir_built` fed with the output of the `ByMoveBody` MIR transformation, and some other relevant queries. * Remove the special `PassManager` hacks for handling the inner `by_move_body` stored within the coroutine's mir body. Instead, this body is fed like a regular MIR body, so it's goes through all of the `tcx.*_mir` stages normally (build -> promoted -> ...etc... -> optimized) ✨. * Introduce `TyCtxt::is_synthetic_mir` to skip over `mir_borrowck` which is called by `mir_promoted`; borrowck isn't really possible to make work ATM since it heavily relies being called on a body generated from HIR, and is redundant by the construction of the by-move-body. * Remove the `InstanceKind::ByMoveBody` shim, since now we have a "regular" def id, we can just use `InstanceKind::Item`. This also allows us to remove the corresponding hacks from codegen, such as in `fn_sig_for_fn_abi` ✨. Notable remarks: * I know it's kind of weird to be using `DefKind::Closure` here, since it's not a distinct closure but just a new MIR body. I don't believe it really matters, but I could also use a different `DefKind`... maybe one that we could use for synthetic MIR bodies in general?

bors · 2024-08-01T17:26:29Z

⌛ Trying commit 36ed446 with merge 3904d97...

bors · 2024-08-01T19:22:52Z

☀️ Try build successful - checks-actions
Build commit: 3904d97 (3904d9726c8057476f260c7c166008d0eb4f50e4)

rust-timer · 2024-08-02T14:56:58Z

Finished benchmarking commit (3904d97): 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.4%	[0.2%, 0.8%]	54
Regressions ❌ (secondary)	0.7%	[0.2%, 1.1%]	18
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 0.8%]	54

Max RSS (memory usage)

Results (primary 5.4%, secondary 8.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)	5.4%	[0.6%, 16.9%]	7
Regressions ❌ (secondary)	8.5%	[6.8%, 11.8%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	5.4%	[0.6%, 16.9%]	7

Cycles

Results (primary 1.7%)

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.7%	[1.2%, 2.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.7%	[1.2%, 2.1%]	2

Binary size

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

Bootstrap: 756.054s -> 761.062s (0.66%)
Artifact size: 336.95 MiB -> 337.01 MiB (0.02%)

compiler-errors · 2024-08-02T15:56:19Z

If this doesn't help things, I'll investigate adding a new DefKind::SyntheticBody or something, to avoid needing to add tcx.is_synthetic_mir as a query.

compiler-errors · 2024-08-02T15:56:27Z

@bors try @rust-timer queue

bors · 2024-08-02T15:57:40Z

⌛ Trying commit bca0ea9 with merge a92413e...

bors · 2024-08-27T13:50:32Z

💔 Test failed - checks-actions

compiler-errors · 2024-08-27T13:54:39Z

windows should stop being a supported operating system tbh

@bors retry

bors · 2024-08-27T17:00:06Z

⌛ Testing commit 93295ff with merge ba2f204...

…llot Stop storing a special inner body for the coroutine by-move body for async closures ...and instead, just synthesize an item which is treated mostly normally by the MIR pipeline. This PR does a few things: * We synthesize a new `DefId` for the by-move body of a closure, which has its `mir_built` fed with the output of the `ByMoveBody` MIR transformation, and some other relevant queries. * This has the `DefKind::ByMoveBody`, which we use to distinguish it from "real" bodies (that come from HIR) which need to be borrowck'd. Introduce `TyCtxt::is_synthetic_mir` to skip over `mir_borrowck` which is called by `mir_promoted`; borrowck isn't really possible to make work ATM since it heavily relies being called on a body generated from HIR, and is redundant by the construction of the by-move-body. * Remove the special `PassManager` hacks for handling the inner `by_move_body` stored within the coroutine's mir body. Instead, this body is fed like a regular MIR body, so it's goes through all of the `tcx.*_mir` stages normally (build -> promoted -> ...etc... -> optimized) ✨. * Remove the `InstanceKind::ByMoveBody` shim, since now we have a "regular" def id, we can just use `InstanceKind::Item`. This also allows us to remove the corresponding hacks from codegen, such as in `fn_sig_for_fn_abi` ✨. Notable remarks: * ~~I know it's kind of weird to be using `DefKind::Closure` here, since it's not a distinct closure but just a new MIR body. I don't believe it really matters, but I could also use a different `DefKind`... maybe one that we could use for synthetic MIR bodies in general?~~ edit: We're doing this now.

rust-log-analyzer · 2024-08-27T18:27:33Z

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

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

[RUSTC-TIMING] miri test:false 4.457
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Tue, Aug 27, 2024  6:27:19 PM
  network time: Tue, 27 Aug 2024 18:27:19 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

bors · 2024-08-27T18:28:12Z

💔 Test failed - checks-actions

compiler-errors · 2024-08-27T20:28:10Z

@bors retry

bors · 2024-08-27T23:30:27Z

⌛ Testing commit 93295ff with merge d9a2cc4...

bors · 2024-08-28T02:09:56Z

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

rust-timer · 2024-08-28T03:26:26Z

Finished benchmarking commit (d9a2cc4): 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)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

Results (primary 2.2%, secondary 2.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)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	2.5%	[2.1%, 3.0%]	16
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

Binary size

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

Bootstrap: 752.135s -> 754.016s (0.25%)
Artifact size: 338.76 MiB -> 338.88 MiB (0.03%)

…idtwco Do not call query to compute coroutine layout for synthetic body of async closure There is code in the MIR validator that attempts to prevent query cycles when inlining a coroutine into itself, and will use the coroutine layout directly from the body when it detects that's the same coroutine as the one that's being validated. After rust-lang#128506, this logic didn't take into account the fact that the coroutine def id will differ if it's the "by-move body" of an async closure. This PR implements that. Fixes rust-lang#129811

Rollup merge of rust-lang#129847 - compiler-errors:async-cycle, r=davidtwco Do not call query to compute coroutine layout for synthetic body of async closure There is code in the MIR validator that attempts to prevent query cycles when inlining a coroutine into itself, and will use the coroutine layout directly from the body when it detects that's the same coroutine as the one that's being validated. After rust-lang#128506, this logic didn't take into account the fact that the coroutine def id will differ if it's the "by-move body" of an async closure. This PR implements that. Fixes rust-lang#129811

…=cjgillot Don't use `typeck_root_def_id` in codegen for finding closure's root Generating debuginfo in codegen currently peels off all the closure-specific generics (which presumably is done because they're redundant). This doesn't currently work correctly for the bodies we synthesize for async closures's returned coroutines (rust-lang#128506), leading to rust-lang#129702. Specifically, `typeck_root_def_id` for some `DefKind::SyntheticCoroutineBody` just returns itself (because it loops while `is_typeck_child` is `true`, and that returns `false` for this defkind), which means we don't end up peeling off the coroutine-specific generics, and we end up encountering an otherwise unreachable `CoroutineWitness` type leading to an ICE. This PR fixes `is_typeck_child` to consider `DefKind::SyntheticCorotuineBody` to be a typeck child, fixing `typeck_root_def_id` and suppressing this debuginfo bug. Fixes rust-lang#129702

rustbot assigned chenyukang Aug 1, 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 Aug 1, 2024

This comment was marked as outdated.

Sign in to view

compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2024

compiler-errors commented Aug 1, 2024

View reviewed changes

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 Aug 1, 2024

compiler-errors mentioned this pull request Aug 1, 2024

MIR required_consts, mentioned_items: ensure we do not forget to fill these lists #128494

Merged

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the by-move-body branch from 36ed446 to 776dc68 Compare August 1, 2024 23:05

compiler-errors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2024

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 2, 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 Aug 2, 2024

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 Aug 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 Aug 27, 2024

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 Aug 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 Aug 27, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 28, 2024

bors merged commit d9a2cc4 into rust-lang:master Aug 28, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 28, 2024

bors mentioned this pull request Aug 28, 2024

Async drop codegen #123948

Open

traviscross added the F-async_closure `#![feature(async_closure)]` label Aug 28, 2024

This was referenced Aug 29, 2024

Don't use typeck_root_def_id in codegen for finding closure's root #129716

Merged

Cycle detected when optimizing MIR on nightly #129811

Closed

Do not call query to compute coroutine layout for synthetic body of async closure #129847

Merged

compiler-errors mentioned this pull request Nov 6, 2024

Stabilize async closures (RFC 3668) #132706

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop storing a special inner body for the coroutine by-move body for async closures #128506

Stop storing a special inner body for the coroutine by-move body for async closures #128506

compiler-errors commented Aug 1, 2024 •

edited by fmease

Loading

rustbot commented Aug 1, 2024

rustbot commented Aug 1, 2024

This comment was marked as outdated.

compiler-errors commented Aug 1, 2024

compiler-errors Aug 1, 2024

compiler-errors commented Aug 1, 2024

This comment has been minimized.

bors commented Aug 1, 2024

This comment has been minimized.

bors commented Aug 1, 2024

This comment has been minimized.

rust-timer commented Aug 2, 2024

compiler-errors commented Aug 2, 2024

compiler-errors commented Aug 2, 2024

This comment has been minimized.

bors commented Aug 2, 2024

bors commented Aug 27, 2024

compiler-errors commented Aug 27, 2024

bors commented Aug 27, 2024

rust-log-analyzer commented Aug 27, 2024

bors commented Aug 27, 2024

compiler-errors commented Aug 27, 2024

bors commented Aug 27, 2024

bors commented Aug 28, 2024

rust-timer commented Aug 28, 2024

Stop storing a special inner body for the coroutine by-move body for async closures #128506

Stop storing a special inner body for the coroutine by-move body for async closures #128506

Conversation

compiler-errors commented Aug 1, 2024 • edited by fmease Loading

rustbot commented Aug 1, 2024

rustbot commented Aug 1, 2024

This comment was marked as outdated.

compiler-errors commented Aug 1, 2024

Choose a reason for hiding this comment

compiler-errors commented Aug 1, 2024

This comment has been minimized.

bors commented Aug 1, 2024

This comment has been minimized.

bors commented Aug 1, 2024

This comment has been minimized.

rust-timer commented Aug 2, 2024

Overall result: ❌ regressions - ACTION NEEDED

compiler-errors commented Aug 2, 2024

compiler-errors commented Aug 2, 2024

This comment has been minimized.

bors commented Aug 2, 2024

bors commented Aug 27, 2024

compiler-errors commented Aug 27, 2024

bors commented Aug 27, 2024

rust-log-analyzer commented Aug 27, 2024

bors commented Aug 27, 2024

compiler-errors commented Aug 27, 2024

bors commented Aug 27, 2024

bors commented Aug 28, 2024

rust-timer commented Aug 28, 2024

Overall result: ✅ improvements - no action needed

compiler-errors commented Aug 1, 2024 •

edited by fmease

Loading