Skip to content

Commit f0cbd03

Browse files
committed
UpvarToLocalProp is a MIR optimization for Future size blowup.
Problem Overview ---------------- Consider: `async fn(x: param) { a(&x); b().await; c(&c); }`. It desugars to: `fn(x: param) -> impl Future { async { let x = x; a(&x); b().await; c(&c); }` and within that desugared form, the `let x = x;` ends up occupying *two* distinct slots in the generator: one for the upvar (the right-hand side) and one for the local (the left-hand side). The UpvarToLocalProp MIR transformation tries to detect the scenario where we have a generator with upvars (which look like `self.field` in the MIR) that are *solely* moved/copied to non-self locals (and those non-self locals may likewise be moved/copied to other locals). After identifying the full set of locals that are solely moved/copied from `self.field`, the transformation replaces them all with `self.field` itself. Note 1: As soon as you have a use local L that *isn't* a local-to-local assignment, then that is something you need to respect, and the propagation will stop trying to propagate L at that point. (And likewise, writes to the self-local `_1`, or projections thereof, need to be handled with care as well.) Note 2: `_0` is the special "return place" and should never be replaced with `self.field`. In addition, the UpvarToLocalProp transformation removes all the silly `self.field = self.field;` assignments that result from the above replacement (apparently some later MIR passes try to double-check that you don't have any assignments with overlapping memory, so it ends up being necessary to do this no-op transformation to avoid assertions later). Note 3: This transformation is significantly generalized past what I demonstrated on youtube; the latter was focused on matching solely `_3 = _1.0`, because it was a proof of concept to demostrate that a MIR transformation prepass even *could* address the generator layout problem. Furthermore, the UpvarToLocalProp transformation respects optimization fuel: you can use `-Z fuel=$CRATE=$FUEL` and when the fuel runs out, the transformation will stop being applied, or be applied only partially. Note 4: I did not put the optimization fuel check in the patching code for UpvarToLocalProp: once you decide to replace `_3` with `_1.0` in `_3 = _1.0;`, you are committed to replacing all future reads of `_3` with `_1.0`, and it would have complicated the patch transformation to try to use fuel with that level of control there. Instead, the way I used the fuel was to have it control how many local variables are added to the `local_to_root_upvar_and_ty` table, which is the core database that informs the patching process, and thus gets us the same end effect (of limiting the number of locals that take part in the transformation) in a hopefully sound manner. Note 5: Added check that we do not ever call `visit_local` on a local that is being replaced. This way we hopefully ensure that we will ICE if we ever forget to replace one. But also: I didnt think I needed to recur on place, but failing to do so meant I overlooked locals in the projection. So now I recur on place. Satisfying above two changes did mean we need to be more aggressive about getting rid of now useless StorageLive and StorageDead on these locals. Note 6: Derefs invalidate replacement attempts in any context, not just mutations. Updates ------- rewrote saw_upvar_to_local. Namely, unified invalidation control paths (because I realized that when looking at `_l = _1.field`, if you need to invalidate either left- or right-hand side, you end up needing to invalidate both). Also made logic for initializing the upvar_to_ty map more robust: Instead of asserting that we encounter each upvar at most once (because, when chains stop growing, we cannot assume that), now just ensure that the types we end up inserting are consistent. (Another alternative would be to bail out of the routine if the chain is not marked as growing; I'm still debating about which of those two approaches yields better code here.) Fixed a bug in how I described an invariant on `LocalState::Ineligible`. Updated to respect -Zmir_opt_level=0
1 parent 6277dbb commit f0cbd03

File tree

4 files changed

+714
-33
lines changed

4 files changed

+714
-33
lines changed

compiler/rustc_mir_transform/src/lib.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ mod simplify_comparison_integral;
104104
mod sroa;
105105
mod uninhabited_enum_branching;
106106
mod unreachable_prop;
107+
mod upvar_to_local_prop;
107108

108109
use rustc_const_eval::transform::check_consts::{self, ConstCx};
109110
use rustc_const_eval::transform::promote_consts;
@@ -491,6 +492,10 @@ fn run_runtime_lowering_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
491492
// `AddRetag` needs to run after `ElaborateDrops`. Otherwise it should run fairly late,
492493
// but before optimizations begin.
493494
&elaborate_box_derefs::ElaborateBoxDerefs,
495+
// `UpvarToLocalProp` needs to run before `generator::StateTransform`, because its
496+
// purpose is to coalesce locals into their original upvars before fresh space is
497+
// allocated for them in the generator.
498+
&upvar_to_local_prop::UpvarToLocalProp,
494499
&generator::StateTransform,
495500
&add_retag::AddRetag,
496501
&Lint(const_prop_lint::ConstProp),

0 commit comments

Comments
 (0)