Transform async `ResumeTy` in generator transform #105977

Swatinem · 2022-12-20T21:52:37Z

Eliminates all the get_context calls that async lowering created.
Replace all Local ResumeTy types with &mut Context<'_>.

The Locals that have their types replaced are:

The resume argument itself.
The argument to get_context.
The yielded value of a yield.

The ResumeTy hides a &mut Context<'_> behind an unsafe raw pointer, and the get_context function is being used to convert that back to a &mut Context<'_>.

Ideally the async lowering would not use the ResumeTy/get_context indirection, but rather directly use &mut Context<'_>, however that would currently lead to higher-kinded lifetime errors.
See #105501.

The async lowering step and the type / lifetime inference / checking are still using the ResumeTy indirection for the time being, and that indirection is removed here. After this transform, the generator body only knows about &mut Context<'_>.

Fixes https://github.com/bjorn3/rustc_codegen_cranelift/issues/1330 CC @bjorn3

r? @compiler-errors

rustbot · 2022-12-20T21:52:47Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2022-12-21T18:20:49Z

I'm probably not the best person to review mir-opt work.

r? @rust-lang/wg-mir-opt

bors · 2022-12-29T05:19:07Z

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

eddyb · 2023-01-02T01:14:27Z

@rustbot assigned @eddyb and unassigned @compiler-errors (2 weeks ago)

Uhh that's a bug I'm pretty sure? I shouldn't be given any reviews (cc @oli-obk).

r? @rust-lang/wg-mir-opt

oli-obk · 2023-01-09T11:33:41Z

Uhh that's a bug I'm pretty sure? I shouldn't be given any review

explicit team assignments will pick anyone from that team, even if they are not on the automatic rotation

spastorino · 2023-01-16T21:15:30Z

r? @rust-lang/wg-mir-opt

compiler/rustc_mir_transform/src/generator.rs

oli-obk · 2023-01-17T08:39:30Z

compiler/rustc_mir_transform/src/generator.rs

+fn transform_async_context<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    body: &mut Body<'tcx>,
+    context_mut_ref: Ty<'tcx>,
+) {


I wonder if we could register this as a special mir opt so we can see its diff in a mir opt test. Seems like a lot of work though, and out of scope for this PR

That would be interesting, but not sure if its worth it.

I consider this PR just a temporary workaround that I would love to revert as soon as the compiler can handle &mut Context<'_> in generators directly.

oli-obk · 2023-01-17T16:42:38Z

@bors r+

bors · 2023-01-17T16:42:40Z

📌 Commit 1db48e2 has been approved by oli-obk

It is now in the queue for this repository.

Transform async `ResumeTy` in generator transform - Eliminates all the `get_context` calls that async lowering created. - Replace all `Local` `ResumeTy` types with `&mut Context<'_>`. The `Local`s that have their types replaced are: - The `resume` argument itself. - The argument to `get_context`. - The yielded value of a `yield`. The `ResumeTy` hides a `&mut Context<'_>` behind an unsafe raw pointer, and the `get_context` function is being used to convert that back to a `&mut Context<'_>`. Ideally the async lowering would not use the `ResumeTy`/`get_context` indirection, but rather directly use `&mut Context<'_>`, however that would currently lead to higher-kinded lifetime errors. See <rust-lang#105501>. The async lowering step and the type / lifetime inference / checking are still using the `ResumeTy` indirection for the time being, and that indirection is removed here. After this transform, the generator body only knows about `&mut Context<'_>`. --- Fixes https://github.com/bjorn3/rustc_codegen_cranelift/issues/1330 CC `@bjorn3` r? `@compiler-errors`

compiler-errors · 2023-01-18T22:59:39Z

Needs mir-opt tests blessed: #107046 (comment)

@bors r-

- Eliminates all the `get_context` calls that async lowering created. - Replace all `Local` `ResumeTy` types with `&mut Context<'_>`. The `Local`s that have their types replaced are: - The `resume` argument itself. - The argument to `get_context`. - The yielded value of a `yield`. The `ResumeTy` hides a `&mut Context<'_>` behind an unsafe raw pointer, and the `get_context` function is being used to convert that back to a `&mut Context<'_>`. Ideally the async lowering would not use the `ResumeTy`/`get_context` indirection, but rather directly use `&mut Context<'_>`, however that would currently lead to higher-kinded lifetime errors. See <rust-lang#105501>. The async lowering step and the type / lifetime inference / checking are still using the `ResumeTy` indirection for the time being, and that indirection is removed here. After this transform, the generator body only knows about `&mut Context<'_>`.

Swatinem · 2023-01-19T08:05:00Z

Rebased, and added compile-flags: -C panic=abort to the test.
The problem was that the wasm32 target defaults to panic=abort and thus no unwinding paths and future polled after panic was generated there.
Using panic=abort directly should make the output consistent across targets and even simpler.

oli-obk · 2023-01-19T08:08:45Z

@bors r+

bors · 2023-01-19T08:08:48Z

📌 Commit 96931a7 has been approved by oli-obk

It is now in the queue for this repository.

…llaumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#105977 (Transform async `ResumeTy` in generator transform) - rust-lang#106927 (make `CastError::NeedsDeref` create a `MachineApplicable` suggestion) - rust-lang#106931 (document + UI test `E0208` and make its output more user-friendly) - rust-lang#107027 (Remove extra removal from test path) - rust-lang#107037 (Fix Dominators::rank_partial_cmp to match documentation) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bjorn3 · 2023-01-20T16:06:57Z

Updated cg_clif in bjorn3/rustc_codegen_cranelift@6c58be8. Works perfect!

to overcome an issue with async generators. Updated the codegen_generator to follow the new logic implemented in rust-lang/rust#105977

This changes from the stdlib supported `ResumeTy`, and converting that to a `&mut Context<'_>` to directly use `&mut Context<'_>` in lowering. It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250. The PR is currently failing as it depends on `-Zdrop-tracking-mir` becoming the default, so that the compiler does not falsly believe the context is being held across await points,which would make async blocks `!Send` and `!UnwindSafe`. However it also still fails the testcase added in rust-lang#106264 for reasons I don’t understand.

Upgrade our toolchain to `nightly-2023-01-23`. The changes here are related to the following changes: - rust-lang/rust#104986 - rust-lang/rust#105657 - rust-lang/rust#105603 - rust-lang/rust#105613 - rust-lang/rust#105977 - rust-lang/rust#104645

Instead of using the stdlib supported `ResumeTy`, which is being converting to a `&mut Context<'_>` during the Generator MIR pass, this will use `&mut Context<'_>` directly in HIR lowering. It pretty much reverts rust-lang#105977 and re-applies an updated version of rust-lang#105250. This still fails the testcase added in rust-lang#106264 however, for reasons I don’t understand.

rustbot assigned compiler-errors Dec 20, 2022

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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 20, 2022

rustbot assigned eddyb and unassigned compiler-errors Dec 21, 2022

Noratrieb removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 27, 2022

Swatinem force-pushed the async-mir-context branch from 959d552 to 238bd05 Compare December 29, 2022 09:05

rustbot assigned spastorino and unassigned eddyb Jan 2, 2023

bjorn3 mentioned this pull request Jan 8, 2023

Consider enabling "Releases" on GitHub rust-lang/rustc_codegen_cranelift#1337

Closed

bjorn3 mentioned this pull request Jan 13, 2023

Wire up testing to ./x.py test in rust-lang/rust rust-lang/rustc_codegen_cranelift#1290

Open

13 tasks

rustbot assigned oli-obk and unassigned spastorino Jan 16, 2023

Swatinem force-pushed the async-mir-context branch from 238bd05 to 332a0fc Compare January 17, 2023 08:26

oli-obk reviewed Jan 17, 2023

View reviewed changes

compiler/rustc_mir_transform/src/generator.rs Outdated Show resolved Hide resolved

oli-obk reviewed Jan 17, 2023

View reviewed changes

Swatinem force-pushed the async-mir-context branch from 332a0fc to 1db48e2 Compare January 17, 2023 16:25

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 Jan 17, 2023

matthiaskrgr mentioned this pull request Jan 18, 2023

Rollup of 7 pull requests #107046

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 18, 2023

Swatinem force-pushed the async-mir-context branch from 1db48e2 to 96931a7 Compare January 19, 2023 08:03

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 19, 2023

GuillaumeGomez mentioned this pull request Jan 19, 2023

Rollup of 5 pull requests #107064

Merged

bors merged commit 30ddeef into rust-lang:master Jan 19, 2023

rustbot added this to the 1.68.0 milestone Jan 19, 2023

Swatinem deleted the async-mir-context branch January 19, 2023 14:07

celinval added a commit to celinval/kani-dev that referenced this pull request Jan 24, 2023

Upgrade toolchain to nightly-2023-01-23

c5db1df

to overcome an issue with async generators. Updated the codegen_generator to follow the new logic implemented in rust-lang/rust#105977

celinval mentioned this pull request Jan 24, 2023

Upgrade toolchain to nightly-2023-01-23 model-checking/kani#2149

Merged

4 tasks

Swatinem mentioned this pull request Feb 1, 2023

WIP: Remove ResumeTy from async lowering #107562

Draft

1 task

bjorn3 mentioned this pull request Nov 2, 2023

Implement gen blocks in the 2024 edition #116447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transform async `ResumeTy` in generator transform #105977

Transform async `ResumeTy` in generator transform #105977

Swatinem commented Dec 20, 2022

rustbot commented Dec 20, 2022

compiler-errors commented Dec 21, 2022

bors commented Dec 29, 2022

eddyb commented Jan 2, 2023

oli-obk commented Jan 9, 2023

spastorino commented Jan 16, 2023

oli-obk Jan 17, 2023

Swatinem Jan 17, 2023

oli-obk commented Jan 17, 2023

bors commented Jan 17, 2023

compiler-errors commented Jan 18, 2023

Swatinem commented Jan 19, 2023

oli-obk commented Jan 19, 2023

bors commented Jan 19, 2023

bjorn3 commented Jan 20, 2023

Transform async ResumeTy in generator transform #105977

Transform async ResumeTy in generator transform #105977

Conversation

Swatinem commented Dec 20, 2022

rustbot commented Dec 20, 2022

compiler-errors commented Dec 21, 2022

bors commented Dec 29, 2022

eddyb commented Jan 2, 2023

oli-obk commented Jan 9, 2023

spastorino commented Jan 16, 2023

oli-obk Jan 17, 2023

Choose a reason for hiding this comment

Swatinem Jan 17, 2023

Choose a reason for hiding this comment

oli-obk commented Jan 17, 2023

bors commented Jan 17, 2023

compiler-errors commented Jan 18, 2023

Swatinem commented Jan 19, 2023

oli-obk commented Jan 19, 2023

bors commented Jan 19, 2023

bjorn3 commented Jan 20, 2023

Transform async `ResumeTy` in generator transform #105977

Transform async `ResumeTy` in generator transform #105977