Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] mir-opt for generators/futures: copy propagate upvar into locals #108590

Commits on Aug 17, 2023

  1. refactoring: move replace_base utility function to root of rustc_mi…

    …r_transform crate.
    
    incorporated review feedback: simplify replace_base via Place::project_deeper
    pnkfelix committed Aug 17, 2023
    Configuration menu
    Copy the full SHA
    6277dbb View commit details
    Browse the repository at this point in the history

Commits on Aug 18, 2023

  1. 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
    pnkfelix committed Aug 18, 2023
    Configuration menu
    Copy the full SHA
    f0cbd03 View commit details
    Browse the repository at this point in the history
  2. InlineFutureIntoFuture is a peephole optimization improving UpvarToLo…

    …calProp.
    
    Problem Overview
    ----------------
    
    When `async fn` is desugared, there's a whole bunch of local-to-local moves that
    are easily identified and eliminated. However, there is one exception: the
    sugaring of `.await` does `a = IntoFuture::into_future(b);`, and that is no longer obviously a move from the viewpoint of the analysis.
    
    However, for all F: Future, `<F as IntoFuture>::into_future(self)` is
    "guaranteed" to be the identity function that returns `self`.
    
    So: this matches `a = <impl Future as IntoFuture>::into_future(b);` and replaces
    it with `a = b;`, based on reasoning that libcore's blanket implementation of
    IntoFuture for impl Future is an identity function that takes `self` by value.
    
    This transformation, in tandem with UpvarToLocalProp, is enough to address both
    case 1 and case 2 of Rust issue 62958.
    
    InlineFutureIntoFuture respects optimization fuel, same as UpvarToLocalProp
    (much simpler to implement in this case).
    
    inline-future-into-future: improved comments during code walk for a rubber duck.
    
    MERGEME inline_future_into_future revised internal instrumentation to print out arg0 type
    
    (because that is what is really going to matter and I should be doing more to
    let it drive the analysis.)
    
    Updates
    -------
    
    respect -Zmir_opt_level=0
    pnkfelix committed Aug 18, 2023
    Configuration menu
    Copy the full SHA
    e88e117 View commit details
    Browse the repository at this point in the history
  3. Updated the future-sizes tests to opt-into the MIR transformations (r…

    …egardless
    
    of their default settings) and then updated the numbers to reflect the
    improvements those transformations now yield.
    pnkfelix committed Aug 18, 2023
    Configuration menu
    Copy the full SHA
    83e15f8 View commit details
    Browse the repository at this point in the history
  4. Updated the print-type-sizes async.rs test to opt into the new MIR

    transformations added here (regardless of their default settings) and updated
    the numbers in the output to reflect the improvements those transformations
    yield.
    pnkfelix committed Aug 18, 2023
    Configuration menu
    Copy the full SHA
    6dbe78d View commit details
    Browse the repository at this point in the history
  5. Some local unit tests to track progress and capture interesting cases…

    … as I identify them.
    
    issue-62958-c.rs was reduced from the tracing-attributes proc-macro crate.
    
    issue-62958-d.rs was reduced from the doc test attached to `AtomicPtr::from_mut_slice`.
    
    issue-62958-e.rs covers some important operational characteristics.
    pnkfelix committed Aug 18, 2023
    Configuration menu
    Copy the full SHA
    f4a1d4b View commit details
    Browse the repository at this point in the history