Skip to content

Commit

Permalink
Auto merge of #128506 - compiler-errors:by-move-body, r=<try>
Browse files Browse the repository at this point in the history
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?
  • Loading branch information
bors committed Aug 1, 2024
2 parents e60ebb2 + 36ed446 commit 3904d97
Show file tree
Hide file tree
Showing 24 changed files with 188 additions and 230 deletions.
1 change: 0 additions & 1 deletion compiler/rustc_const_eval/src/interpret/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,6 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
| ty::InstanceKind::ReifyShim(..)
| ty::InstanceKind::ClosureOnceShim { .. }
| ty::InstanceKind::ConstructCoroutineInClosureShim { .. }
| ty::InstanceKind::CoroutineKindShim { .. }
| ty::InstanceKind::FnPtrShim(..)
| ty::InstanceKind::DropGlue(..)
| ty::InstanceKind::CloneShim(..)
Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_hir_analysis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,6 @@ pub fn check_crate(tcx: TyCtxt<'_>) {
}
});

// Freeze definitions as we don't add new ones at this point. This improves performance by
// allowing lock-free access to them.
tcx.untracked().definitions.freeze();

// FIXME: Remove this when we implement creating `DefId`s
// for anon constants during their parents' typeck.
// Typeck all body owners in parallel will produce queries
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,13 @@ fn run_required_analyses(tcx: TyCtxt<'_>) {
);
}
});
// Freeze definitions as we don't add new ones at this point.
// We need to wait until now since we synthesize a by-move body
// This improves performance by allowing lock-free access to them.
// FIXME(async_closures): We could force `coroutine_by_move_body_def_id`
// immediately after typeck, then freeze after that.
tcx.untracked().definitions.freeze();

sess.time("layout_testing", || layout_test::test_layout(tcx));
sess.time("abi_testing", || abi_test::test_abi(tcx));
}
Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,11 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
let def_span = tcx.def_span(local_id);
record!(self.tables.def_span[def_id] <- def_span);
}
if should_encode_attrs(def_kind) {
// FIXME(async_closures): We should just use `tcx.attrs` rather than going
// through the HIR. Historically, though, this has been inefficient apparently.
// For now, it's kind of pointless to fix, because coroutine-closures' coroutine
// bodies have no attrs anyways.
if should_encode_attrs(def_kind) && !tcx.is_synthetic_mir(local_id) {
self.encode_attrs(local_id);
}
if should_encode_expn_that_defined(def_kind) {
Expand Down
17 changes: 0 additions & 17 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,18 +267,6 @@ pub struct CoroutineInfo<'tcx> {
/// Coroutine drop glue. This field is populated after the state transform pass.
pub coroutine_drop: Option<Body<'tcx>>,

/// The body of the coroutine, modified to take its upvars by move rather than by ref.
///
/// This is used by coroutine-closures, which must return a different flavor of coroutine
/// when called using `AsyncFnOnce::call_once`. It is produced by the `ByMoveBody` pass which
/// is run right after building the initial MIR, and will only be populated for coroutines
/// which come out of the async closure desugaring.
///
/// This body should be processed in lockstep with the containing body -- any optimization
/// passes, etc, should be applied to this body as well. This is done automatically if
/// using `run_passes`.
pub by_move_body: Option<Body<'tcx>>,

/// The layout of a coroutine. This field is populated after the state transform pass.
pub coroutine_layout: Option<CoroutineLayout<'tcx>>,

Expand All @@ -298,7 +286,6 @@ impl<'tcx> CoroutineInfo<'tcx> {
coroutine_kind,
yield_ty: Some(yield_ty),
resume_ty: Some(resume_ty),
by_move_body: None,
coroutine_drop: None,
coroutine_layout: None,
}
Expand Down Expand Up @@ -663,10 +650,6 @@ impl<'tcx> Body<'tcx> {
self.coroutine.as_ref().and_then(|coroutine| coroutine.coroutine_drop.as_ref())
}

pub fn coroutine_by_move_body(&self) -> Option<&Body<'tcx>> {
self.coroutine.as_ref()?.by_move_body.as_ref()
}

#[inline]
pub fn coroutine_kind(&self) -> Option<CoroutineKind> {
self.coroutine.as_ref().map(|coroutine| coroutine.coroutine_kind)
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_middle/src/mir/mono.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,6 @@ impl<'tcx> CodegenUnit<'tcx> {
| InstanceKind::Virtual(..)
| InstanceKind::ClosureOnceShim { .. }
| InstanceKind::ConstructCoroutineInClosureShim { .. }
| InstanceKind::CoroutineKindShim { .. }
| InstanceKind::DropGlue(..)
| InstanceKind::CloneShim(..)
| InstanceKind::ThreadLocalShim(..)
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,6 @@ macro_rules! make_mir_visitor {
coroutine_closure_def_id: _def_id,
receiver_by_ref: _,
} |
ty::InstanceKind::CoroutineKindShim { coroutine_def_id: _def_id } |
ty::InstanceKind::AsyncDropGlueCtorShim(_def_id, None) |
ty::InstanceKind::DropGlue(_def_id, None) => {}

Expand Down
20 changes: 20 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ rustc_queries! {
query predicates_of(key: DefId) -> ty::GenericPredicates<'tcx> {
desc { |tcx| "computing predicates of `{}`", tcx.def_path_str(key) }
cache_on_disk_if { key.is_local() }
feedable
}

query opaque_types_defined_by(
Expand Down Expand Up @@ -488,13 +489,19 @@ rustc_queries! {
separate_provide_extern
}

query is_synthetic_mir(key: LocalDefId) -> bool {
desc { |tcx| "checking if `{}` is synthetic", tcx.def_path_str(key) }
feedable
}

/// Build the MIR for a given `DefId` and prepare it for const qualification.
///
/// See the [rustc dev guide] for more info.
///
/// [rustc dev guide]: https://rustc-dev-guide.rust-lang.org/mir/construction.html
query mir_built(key: LocalDefId) -> &'tcx Steal<mir::Body<'tcx>> {
desc { |tcx| "building MIR for `{}`", tcx.def_path_str(key) }
feedable
}

/// Try to build an abstract representation of the given constant.
Expand Down Expand Up @@ -739,6 +746,7 @@ rustc_queries! {
query constness(key: DefId) -> hir::Constness {
desc { |tcx| "checking if item is const: `{}`", tcx.def_path_str(key) }
separate_provide_extern
feedable
}

query asyncness(key: DefId) -> ty::Asyncness {
Expand All @@ -757,10 +765,22 @@ rustc_queries! {
desc { |tcx| "checking if item is promotable: `{}`", tcx.def_path_str(key) }
}

/// The body of the coroutine, modified to take its upvars by move rather than by ref.
///
/// This is used by coroutine-closures, which must return a different flavor of coroutine
/// when called using `AsyncFnOnce::call_once`. It is produced by the `ByMoveBody` pass which
/// is run right after building the initial MIR, and will only be populated for coroutines
/// which come out of the async closure desugaring.
query coroutine_by_move_body_def_id(def_id: DefId) -> DefId {
desc { |tcx| "looking up the coroutine by-move body for `{}`", tcx.def_path_str(def_id) }
separate_provide_extern
}

/// Returns `Some(coroutine_kind)` if the node pointed to by `def_id` is a coroutine.
query coroutine_kind(def_id: DefId) -> Option<hir::CoroutineKind> {
desc { |tcx| "looking up coroutine kind of `{}`", tcx.def_path_str(def_id) }
separate_provide_extern
feedable
}

query coroutine_for_closure(def_id: DefId) -> DefId {
Expand Down
16 changes: 3 additions & 13 deletions compiler/rustc_middle/src/ty/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,6 @@ pub enum InstanceKind<'tcx> {
receiver_by_ref: bool,
},

/// `<[coroutine] as Future>::poll`, but for coroutines produced when `AsyncFnOnce`
/// is called on a coroutine-closure whose closure kind greater than `FnOnce`, or
/// similarly for `AsyncFnMut`.
///
/// This will select the body that is produced by the `ByMoveBody` transform, and thus
/// take and use all of its upvars by-move rather than by-ref.
CoroutineKindShim { coroutine_def_id: DefId },

/// Compiler-generated accessor for thread locals which returns a reference to the thread local
/// the `DefId` defines. This is used to export thread locals from dylibs on platforms lacking
/// native support.
Expand Down Expand Up @@ -248,7 +240,6 @@ impl<'tcx> InstanceKind<'tcx> {
coroutine_closure_def_id: def_id,
receiver_by_ref: _,
}
| ty::InstanceKind::CoroutineKindShim { coroutine_def_id: def_id }
| InstanceKind::DropGlue(def_id, _)
| InstanceKind::CloneShim(def_id, _)
| InstanceKind::FnPtrAddrShim(def_id, _)
Expand All @@ -270,7 +261,6 @@ impl<'tcx> InstanceKind<'tcx> {
| InstanceKind::Intrinsic(..)
| InstanceKind::ClosureOnceShim { .. }
| ty::InstanceKind::ConstructCoroutineInClosureShim { .. }
| ty::InstanceKind::CoroutineKindShim { .. }
| InstanceKind::DropGlue(..)
| InstanceKind::AsyncDropGlueCtorShim(..)
| InstanceKind::CloneShim(..)
Expand Down Expand Up @@ -377,7 +367,6 @@ impl<'tcx> InstanceKind<'tcx> {
| InstanceKind::AsyncDropGlueCtorShim(_, Some(_)) => false,
InstanceKind::ClosureOnceShim { .. }
| InstanceKind::ConstructCoroutineInClosureShim { .. }
| InstanceKind::CoroutineKindShim { .. }
| InstanceKind::DropGlue(..)
| InstanceKind::AsyncDropGlueCtorShim(..)
| InstanceKind::Item(_)
Expand Down Expand Up @@ -452,7 +441,6 @@ pub fn fmt_instance(
InstanceKind::FnPtrShim(_, ty) => write!(f, " - shim({ty})"),
InstanceKind::ClosureOnceShim { .. } => write!(f, " - shim"),
InstanceKind::ConstructCoroutineInClosureShim { .. } => write!(f, " - shim"),
InstanceKind::CoroutineKindShim { .. } => write!(f, " - shim"),
InstanceKind::DropGlue(_, None) => write!(f, " - shim(None)"),
InstanceKind::DropGlue(_, Some(ty)) => write!(f, " - shim(Some({ty}))"),
InstanceKind::CloneShim(_, ty) => write!(f, " - shim({ty})"),
Expand Down Expand Up @@ -850,7 +838,9 @@ impl<'tcx> Instance<'tcx> {
Some(Instance { def: ty::InstanceKind::Item(coroutine_def_id), args })
} else {
Some(Instance {
def: ty::InstanceKind::CoroutineKindShim { coroutine_def_id },
def: ty::InstanceKind::Item(
tcx.coroutine_by_move_body_def_id(coroutine_def_id),
),
args,
})
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1755,7 +1755,6 @@ impl<'tcx> TyCtxt<'tcx> {
| ty::InstanceKind::Virtual(..)
| ty::InstanceKind::ClosureOnceShim { .. }
| ty::InstanceKind::ConstructCoroutineInClosureShim { .. }
| ty::InstanceKind::CoroutineKindShim { .. }
| ty::InstanceKind::DropGlue(..)
| ty::InstanceKind::CloneShim(..)
| ty::InstanceKind::ThreadLocalShim(..)
Expand Down Expand Up @@ -1873,7 +1872,8 @@ impl<'tcx> TyCtxt<'tcx> {
identity_kind_ty.to_opt_closure_kind(),
Some(ClosureKind::Fn | ClosureKind::FnMut)
);
mir.coroutine_by_move_body().unwrap().coroutine_layout_raw()
self.optimized_mir(self.coroutine_by_move_body_def_id(def_id))
.coroutine_layout_raw()
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/coroutine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
mod by_move_body;
use std::{iter, ops};

pub use by_move_body::ByMoveBody;
pub use by_move_body::coroutine_by_move_body_def_id;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::pluralize;
use rustc_hir as hir;
Expand Down
Loading

0 comments on commit 3904d97

Please sign in to comment.