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

pnkfelix · 2023-03-01T03:52:15Z

This is a pair of targeted MIR optimizations that are intended to address issue #62958, at least partially.

There remain scenarios where one can see upvar/local duplication that are still not covered by this
PR, but this catches the easy ones in a relatively straight-forward way.

Todo:

respect -Z mir-opt-level=0 (comment)
look into -Z mir-enable-passes for toggling, rather than adding new -Z flags (comment)
use revisions in test to check with transformations on and off (comment)
see if upvar_to_local_prop can be removed or simplified by leveraging the existing copy_prop mir transform that is already turned on (but at a different phase, IIUC). (comment)
simplify replace_base via Place::project_deeper (comment)
simplify inline_impl_future_into_future by mimicking the Inliner code (comment)
~~leverage tcx.type_implements_trait method (comment)~~

rustbot · 2023-03-01T03:52:22Z

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-03-01T03:52:26Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

pnkfelix · 2023-03-01T03:58:29Z

Oh, I just remembered: the PR as it currently stands has these two optimizations turned off by default, but my actual plan/hope here is to have them turned on by default (even when other MIR optimizations are not running).

I probably need to talk to a slew of people to get approval to do that. :)

pnkfelix · 2023-03-01T04:02:35Z

I'm going to add a commit that turns the optimizations on by default, for a timer run (and ... maybe a crater run...?)

pnkfelix · 2023-03-01T04:32:06Z

ugh, something's wrong. I had thought I had bootstrapped with these on by default locally, but clearly not recently enough. I'll look at it again tomorrow.

saethlin · 2023-03-01T04:51:50Z

my actual plan/hope here is to have them turned on by default (even when other MIR optimizations are not running).

We have -Zmir-opt-level=0 which Miri passes to turn off MIR optimizations. Miri needs this because optimizations often exploit UB and end up turning programs which encounter UB into faster ones that don't.

Is there particular reasoning you think these optimizations should be treated differently? Is there some reason you're sure these will never remove/hide UB from Miri?

tests/ui/async-await/future-sizes/async-awaiting-fut.rs

pnkfelix · 2023-03-01T16:02:12Z

@saethlin wrote:

my actual plan/hope here is to have them turned on by default (even when other MIR optimizations are not running).

We have -Zmir-opt-level=0 which Miri passes to turn off MIR optimizations. Miri needs this because optimizations often exploit UB and end up turning programs which encounter UB into faster ones that don't.

Is there particular reasoning you think these optimizations should be treated differently? Is there some reason you're sure these will never remove/hide UB from Miri?

The reason I am inclined to treat these ~~optimizations~~ transformations differently from usual MIR optimizations is that they, by design, have a big effect on the generator layout. To the extent that can manifest itself, in certain scenarios, as the difference between exponential blowup in size versus O(n) size (where n is the number of futures being chained togehter, as illustrated in #62958).

However, maybe -Zmir-opt-level=0 is a reasonable thing for me to respect here. My main concern is that I want these passes to run even if one doesn't pass -O to rustc itself; that's a totally different concern than what you're talking about with miri and detecting UB.

pnkfelix · 2023-03-01T16:06:46Z

I'll switch this to S-waiting-on-author until I've addressed the comments so far. When I've finished with that, I'll remove the "[WIP]" from the title (and put the labels back the way they were)

@rustbot label: +S-waiting-on-author -S-waiting-on-review

pnkfelix · 2023-03-01T16:15:01Z

However, maybe -Zmir-opt-level=0 is a reasonable thing for me to respect here. My main concern is that I want these passes to run even if one doesn't pass -O to rustc itself; that's a totally different concern than what you're talking about with miri and detecting UB.

Okay, I just read the code in Session::mir_opt_level and I can see that mir-opt-level=0 looks like the right criteria for me here.

That is, if I understand correctly:

mir-opt-level=0: has to be opted into via the corresponding -Z flag
mir-opt-level=1: default for -O0 aka -C optimize=no
mir-opt-level=2: default for -Onfor n > 0, aka if one chooses any -C optimize level other than No

And mir-opt-level > 0 sounds like exactly like what I want, assuming that its reasonable to potentially force miri to deal with the future size explosion.

cjgillot

Just a few hints on prior work that may help simplify the implementation. I haven't started reading the UpvarToLocalProp pass, as it's quite a big one.

The big question is the implications of the MIR opsem for generators. On this, @JakobDegen will be the better person to ask.

If we are going to add passes to optimize generators prior to the state transform, we should eventually consider moving the state transform back to running on optimized MIR.

cjgillot · 2023-03-01T19:17:09Z

compiler/rustc_mir_transform/src/inline_future_into_future.rs

+            mir_source_def_id,
+            local_decls,
+        };
+        v.visit_body(body);


We don't really need a visitor here, a simple loop over basic blocks looking at terminators should be enough.

cjgillot · 2023-03-01T19:19:08Z

compiler/rustc_mir_transform/src/inline_future_into_future.rs

+        let Operand::Constant(c) = func else { return None; };
+        let ConstantKind::Val(val_const, const_ty) = c.literal else { return None; };
+        let ConstValue::ZeroSized = val_const else { return None; };
+        let ty::FnDef(fn_def_id, substs) =  const_ty.kind() else { return None; };


We only need to check that func.ty(local_decls, tcx).kind() is a FnDef. We don't need to check that it's actually a constant.

cjgillot · 2023-03-01T19:24:18Z

compiler/rustc_mir_transform/src/inline_future_into_future.rs

+        let arg0_ty = args.get(0).map(|arg0| arg0.ty(&self.local_decls, self.tcx()));
+        trace!("InlineFutureIntoFuture substs:{substs:?} args:{args:?} arg0 ty:{arg0_ty:?}");
+        let Some(arg0_ty) = arg0_ty else { return None; };
+        found.0 = self.does_ty_impl_future(arg0_ty);


This code looks like we are re-implementing a simplified version of instance resolution.
What about doing the same thing as the Inliner optimization:

let substs = tcx.try_normalize_erasing_regions(param_env, substs).ok()?; let callee = Instance::resolve(tcx, param_env, def_id, substs).flatten()?;

and check that callee.def is InstanceDef::Item(<def_id of the default impl of IntoFuture::into_future>).

compiler/rustc_mir_transform/src/lib.rs

compiler/rustc_mir_transform/src/inline_future_into_future.rs

cjgillot · 2023-03-01T19:29:55Z

compiler/rustc_mir_transform/src/upvar_to_local_prop.rs

+            } else {
+                // otherwise, fall back to scanning code to find "upvar" count.
+                struct MaxUpvarField(usize);
+                impl<'tcx> Visitor<'tcx> for MaxUpvarField {


Should we restrict this pass to only generators or closures?

I'd be happy to restrict it to generators, at least for turning it on by default.

(but it certainly has been useful to be able to at least opt-into the more general case, in terms of finding new bugs.)

cjgillot · 2023-03-01T19:31:21Z

compiler/rustc_mir_transform/src/upvar_to_local_prop.rs

+            // If we are looking at a generator, we have a fast path
+            if body.yield_ty().is_some() {
+                // The first argument is the generator type passed by value
+                let gen_ty = body.local_decls.raw[SELF_IDX as usize].ty;


This may need to peel refs to fetch the actual generator type.

you mean because the generator might be receiving its self argument by something other than by-value Self, right?

I should look into that.

cjgillot · 2023-03-01T19:36:10Z

compiler/rustc_mir_transform/src/upvar_to_local_prop.rs

+//!   about array indices nor enum variants).
+//!
+//! * transitively apply the replacement to subsequent copies of the local(s).
+//!   e.g. `_3 = _1.0; ... _4 = _3; ...; _5 = _4;`, then might replace `_5` with `_1.0`.


This looks a lot like what CopyProp does. (That pass is very poorly named, as it merges locals more than anything.)
There may be a way to simplify the implementation by using the same analysis to unify locals, and a second pass to replace the locals by the upvars.

Yes, its possible there's opportunity for simplification there.

Part of my goal here was to have this be an inherently cheap pass. (For example, I wanted to avoid any question of whether there might be a fixed point iteration going on.)

But if CopyProp is sufficiently cheap to run that we might consider turning it on for mir-opt-level > 0, I'd be willing to consider unifying these things.

(I also was hoping for this to be an "obviously correct" pass, by building in limitations like the fact that it solely handles trivial chains A -> B -> C, no branching allowed for any of the chains. Though the fact that I encountered a number of issues in its development has led me to think that maybe there's no such thing as "obviously correct" when it comes to MIR transformations... 🤔 )

FYI CopyProp is already so enabled.

It sounds like you are realizing that MIR is not a good IR to write optimizations for, which is a very common observation.

It sounds like you are realizing that MIR is not a good IR to write optimizations for, which is a very common observation.

Well... I've seen much much worse. :)

But if CopyProp is sufficiently cheap to run that we might consider turning it on for mir-opt-level > 0, I'd be willing to consider unifying these things.

I've added investigation of this option as a to-do for the PR. (I.e., I'm going to attempt to revise upvar_to_local_prop so that it leverages CopyProp+SSA code in some way. Either it works and this PR gets vastly simpler, or it doesn't work and I provide an explanation for why to continue using the code as written here.)

saethlin · 2023-03-01T20:50:28Z

assuming that its reasonable to potentially force miri to deal with the future size explosion.

I think it is reasonable. Miri already executes code without optimizations that are always present in a debug build, so categorically this is not new. Additionally, anyone who has run Miri on a program beyond a unit test is familiar with the idea that it is very slow and memory-hungry. Even on unit tests, it's common for users to #[cfg(miri)] to reduce the workload under Miri. (until a few months ago, Miri could consume unbounded memory when executing a program which when run in a normal debug build consumed a fixed amount of memory)

And with regard to this optimization in particular: One potential situation that could arise is that a library starts to produce generators which rely on this optimization, to the degree that it encounters stack overflow if this optimization is not applied. Such a situation does not apply in Miri; the interpreter doesn't have separate stack and heap memory and even if it did, there's no reason it needs to have an artificial stack memory usage limit like normal platforms have.

So: I think this is quite reasonable as a MIR opt level 1 optimization.

csmoe · 2023-03-07T08:34:02Z

compiler/rustc_mir_transform/src/inline_future_into_future.rs

+    // This verifies that `ty` implements `Future`, according to the where
+    // clauses (i.e. predicates) attached to the source code identified by
+    // `mir_source_def_id`).
+    fn does_ty_impl_future(&self, ty: Ty<'tcx>) -> FoundImplFuture {


tcx has a type_implements_trait util function, can we invoke it here? Not sure whether type_implements_trait(future,...) and does_ty_impl_future are same or not.
cc

rust/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

Line 3373 in 8824994

let impls_future = self.type_implements_trait(

I have now added as a to-do for the PR

I cannot reuse the type_implements_trait function; that is a method on the InferCtxt, not on TyCtxt (and I only have a TyCtxt in the context of this code).

pnkfelix · 2023-03-08T18:01:36Z

Just a quick note that this PR is intended to address at least some of the cases listed on issue #59087, though I still need to dig into whether the misgivings about unsafe code are relevant even for the limited transformation being applied here. (T-opsem has certainly expressed some misgivings.)

bors · 2023-04-29T12:16:10Z

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

…r_transform crate. incorporated review feedback: simplify replace_base via Place::project_deeper

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

…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

…egardless of their default settings) and then updated the numbers to reflect the improvements those transformations now yield.

transformations added here (regardless of their default settings) and updated the numbers in the output to reflect the improvements those transformations yield.

… 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.

rust-log-analyzer · 2023-08-18T03:14:43Z

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

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

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:c85c95e3d7251135ab7dc9ce3241c5835cc595a9)
Download action repository 'actions/upload-artifact@v3' (SHA:0b7f8abb1508181956e8e162db84b466c27e18ce)
Complete job name: PR - mingw-check-tidy
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
---
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_9b11ede2-ddfa-4b46-a33e-8acebf118591
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=copy-prop-upvar-to-elim-local-for-62958
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_9b11ede2-ddfa-4b46-a33e-8acebf118591
GITHUB_REF=refs/pull/108590/merge
GITHUB_REF_NAME=108590/merge
GITHUB_REF_PROTECTED=false
---
Removing intermediate container ccc27c8b8995
 ---> cc6bd683b6a8
Step 6/10 : COPY host-x86_64/mingw-check/reuse-requirements.txt /tmp/
 ---> 76233254690f
Step 7/10 : RUN pip3 install --no-deps --no-cache-dir --require-hashes -r /tmp/reuse-requirements.txt     && pip3 install virtualenv
Collecting binaryornot==0.4.4
  Downloading binaryornot-0.4.4-py2.py3-none-any.whl (9.0 kB)
Collecting boolean-py==4.0
  Downloading boolean.py-4.0-py3-none-any.whl (25 kB)
---
Building wheels for collected packages: reuse
  Building wheel for reuse (pyproject.toml): started
  Building wheel for reuse (pyproject.toml): finished with status 'done'
  Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=180116 sha256=351235b2326fb4db7a18e257e13ce7896c5f77339521e2c2612e71e154800a19
  Stored in directory: /tmp/pip-ephem-wheel-cache-rlfmjqc7/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
  Attempting uninstall: setuptools
    Found existing installation: setuptools 59.6.0
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
    Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
    Can't uninstall 'setuptools'. No files were found to uninstall.
Successfully installed binaryornot-0.4.4 boolean-py-4.0 chardet-5.1.0 jinja2-3.1.2 license-expression-30.0.0 markupsafe-2.1.1 python-debian-0.1.49 reuse-1.1.0 setuptools-66.0.0
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
Collecting virtualenv
  Downloading virtualenv-20.24.3-py3-none-any.whl (3.0 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.0/3.0 MB 47.6 MB/s eta 0:00:00
Collecting distlib<1,>=0.3.7
  Downloading distlib-0.3.7-py2.py3-none-any.whl (468 kB)
Collecting filelock<4,>=3.12.2
  Downloading filelock-3.12.2-py3-none-any.whl (10 kB)
  Downloading filelock-3.12.2-py3-none-any.whl (10 kB)
Collecting platformdirs<4,>=3.9.1
  Downloading platformdirs-3.10.0-py3-none-any.whl (17 kB)
Installing collected packages: distlib, platformdirs, filelock, virtualenv
Successfully installed distlib-0.3.7 filelock-3.12.2 platformdirs-3.10.0 virtualenv-20.24.3
Removing intermediate container cf3aeb4606bf
 ---> af4dfc51d521
Step 8/10 : COPY host-x86_64/mingw-check/validate-toolstate.sh /scripts/
 ---> e2920e9221c9
 ---> e2920e9221c9
Step 9/10 : COPY host-x86_64/mingw-check/validate-error-codes.sh /scripts/
 ---> ca14fd153b6b
Step 10/10 : ENV SCRIPT TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
Removing intermediate container 3e77503651d1
 ---> d38c72177c03
Successfully built d38c72177c03
Successfully tagged rust-ci:latest
Successfully tagged rust-ci:latest
##[endgroup]
Built container sha256:d38c72177c03dd763308ce2fff2a17aece8f29510f36ba7c1c653334d1baf96f
Uploading finished image to https://ci-caches.rust-lang.org/docker/f76604104d6a6f4aa068dca8679411c51198f6c5562033174b89cf3ceb2f9ecc175231f1257b18e00fa57bb3eed5f53dbcc121e55de0452dda3db18894e27ea0

<botocore.awsrequest.AWSRequest object at 0x7f1d3fde1ed0>
gzip: stdout: Broken pipe
xargs: docker: terminated by signal 13
[CI_JOB_NAME=mingw-check-tidy]
[CI_JOB_NAME=mingw-check-tidy]
---
DirectMap4k:      169920 kB
DirectMap2M:     6121472 kB
DirectMap1G:    12582912 kB
##[endgroup]
Executing TIDY_PRINT_DIFF=1 python2.7 ../x.py test            --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
+ TIDY_PRINT_DIFF=1 python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py:lint
    Finished dev [unoptimized] target(s) in 0.03s
##[endgroup]
downloading https://ci-artifacts.rust-lang.org/rustc-builds-alt/9b4119009eb99d4087042032ad93ac892a63aa4a/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz
extracting /checkout/obj/build/cache/llvm-9b4119009eb99d4087042032ad93ac892a63aa4a-true/rust-dev-nightly-x86_64-unknown-linux-gnu.tar.xz to /checkout/obj/build/x86_64-unknown-linux-gnu/ci-llvm
---
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 27.18s
##[endgroup]
fmt check
##[error]Diff in /checkout/compiler/rustc_mir_transform/src/inline_future_into_future.rs at line 24:
 
 
     fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
-        let Some(into_future_fn_def_id) = tcx.lang_items().into_future_fn() else { return; };
-        let Some(future_trait_def_id) = tcx.lang_items().future_trait() else { return; };
+        let Some(into_future_fn_def_id) = tcx.lang_items().into_future_fn() else {
+        };
+        };
+        let Some(future_trait_def_id) = tcx.lang_items().future_trait() else {
+        };
+        };
         let mir_source_def_id = body.source.def_id();
         trace!("Running InlineFutureIntoFuture on {:?}", body.source);
         let local_decls = body.local_decls().to_owned();
##[error]Diff in /checkout/compiler/rustc_mir_transform/src/inline_future_into_future.rs at line 74:
         let mir_source_predicates = self.tcx.predicates_of(self.mir_source_def_id);
         let predicates = mir_source_predicates.instantiate_identity(self.tcx);
         for pred in &predicates.predicates {
-            let Some(kind) = pred.kind().no_bound_vars() else { continue; };
-            let ty::ClauseKind::Trait(trait_pred) = kind else { continue; };
-            let ty::TraitPredicate { trait_ref, polarity: ty::ImplPolarity::Positive } = trait_pred else { continue };
+            let Some(kind) = pred.kind().no_bound_vars() else {
+            };
+            let ty::ClauseKind::Trait(trait_pred) = kind else {
+                continue;
+            };
+            };
+            let ty::TraitPredicate { trait_ref, polarity: ty::ImplPolarity::Positive } = trait_pred
+                continue;
+            };
 
 
             // FIXME: justify ignoring `substs` below. My current argument is
             // that `trait Future` has no generic parameters, and the blanket
##[error]Diff in /checkout/compiler/rustc_mir_transform/src/inline_future_into_future.rs at line 100:
         self.tcx
     }
     fn visit_basic_block_data(&mut self, _bb: BasicBlock, bb_data: &mut BasicBlockData<'tcx>) {
-        let Some(term) = &mut bb_data.terminator else { return; };
-        let Some(result) = self.analyze_terminator(term) else { return; };
-        let ImplFutureCallingIntoFuture {
-            args, destination: dest, target: Some(target)
-        } = result else { return; };
+        let Some(term) = &mut bb_data.terminator else {
+        };
+        };
+        let Some(result) = self.analyze_terminator(term) else {
+        };
+        };
+        let ImplFutureCallingIntoFuture { args, destination: dest, target: Some(target) } = result
+            return;
+        };
 
         // At this point, we have identified this terminator as a call to the
         // At this point, we have identified this terminator as a call to the
         // associated function `<impl Future as IntoFuture>::into_future`
##[error]Diff in /checkout/compiler/rustc_mir_transform/src/inline_future_into_future.rs at line 111:
         // Due to our knowledge of how libcore implements Future and IntoFuture,
         // we know we can replace such a call with a trivial move.
 
-        let Some(arg0) = args.get(0) else { return; };
+        let Some(arg0) = args.get(0) else {
+        };
 
 
         trace!("InlineFutureIntoFuture bb_data args:{args:?} dest:{dest:?} target:{target:?}");
 
##[error]Diff in /checkout/compiler/rustc_mir_transform/src/inline_future_into_future.rs at line 130:
     ) -> Option<ImplFutureCallingIntoFuture<'tcx>> {
         let mut found = (FoundImplFuture::No, FoundIntoFutureCall::No);
         let &TerminatorKind::Call {
-            ref func, ref args, destination, target, fn_span: _, unwind: _, call_source: _
-        } = &term.kind else { return None; };
-        let Operand::Constant(c) = func else { return None; };
-        let ConstantKind::Val(val_const, const_ty) = c.literal else { return None; };
-        let ConstValue::ZeroSized = val_const else { return None; };
-        let ty::FnDef(fn_def_id, substs) =  const_ty.kind() else { return None; };
+            ref args,
+            destination,
+            target,
+            fn_span: _,
+            fn_span: _,
+            unwind: _,
+            call_source: _,
+        } = &term.kind
+            return None;
+        };
+        };
+        let Operand::Constant(c) = func else {
+        };
+        };
+        let ConstantKind::Val(val_const, const_ty) = c.literal else {
+        };
+        let ConstValue::ZeroSized = val_const else {
+            return None;
+        };
+        };
+        let ty::FnDef(fn_def_id, substs) = const_ty.kind() else {
+        };
+        };
         if *fn_def_id == self.into_future_fn_def_id {
             found.1 = FoundIntoFutureCall::Yes;
         } else {
##[error]Diff in /checkout/compiler/rustc_mir_transform/src/inline_future_into_future.rs at line 144:
         }
         let arg0_ty = args.get(0).map(|arg0| arg0.ty(&self.local_decls, self.tcx()));
         trace!("InlineFutureIntoFuture substs:{substs:?} args:{args:?} arg0 ty:{arg0_ty:?}");
-        let Some(arg0_ty) = arg0_ty else { return None; };
+        let Some(arg0_ty) = arg0_ty else {
+        };
+        };
         found.0 = self.does_ty_impl_future(arg0_ty);
         if let (FoundImplFuture::Yes, FoundIntoFutureCall::Yes) = found {
             trace!("InlineFutureIntoFuture can replace {term:?}, a {func:?} call, with move");
##[error]Diff in /checkout/compiler/rustc_mir_transform/src/upvar_to_local_prop.rs at line 316:
 
 
         // if rhs is valid chain, then must inspect further; otherwise, no more invalidation to do.
-        let LocalState::Valid(rhs_upvar) = self.local_to_upvar[rhs] else { return; };
+        let LocalState::Valid(rhs_upvar) = self.local_to_upvar[rhs] else {
+        };
 
 
         let rhs_chain = &mut self.upvar_to_locals[rhs_upvar];
         assert!(rhs_chain.local_copies.contains(&rhs));
##[error]Diff in /checkout/compiler/rustc_mir_transform/src/upvar_to_local_prop.rs at line 572:
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/rustfmt/bin/rustfmt" "--config-path" "/checkout" "--edition" "2021" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_mir_transform/src/elaborate_box_derefs.rs" "/checkout/compiler/rustc_mir_transform/src/large_enums.rs" "/checkout/compiler/rustc_mir_transform/src/inline_future_into_future.rs" "/checkout/compiler/rustc_mir_transform/src/simplify_comparison_integral.rs" "/checkout/compiler/rustc_mir_transform/src/uninhabited_enum_branching.rs" "/checkout/compiler/rustc_mir_transform/src/deduplicate_blocks.rs" "/checkout/compiler/rustc_mir_transform/src/copy_prop.rs" "/checkout/compiler/rustc_mir_transform/src/required_consts.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
Build completed unsuccessfully in 0:00:40
         let chains = self.upvar_to_locals.iter_enumerated();
         let chains_and_tys = chains.zip(self.upvar_to_ty.vec.into_iter());
         for ((field, chain), ty) in chains_and_tys {
-            let Some(ty) = ty else { assert_eq!(chain.len(), 0); continue; };
+            let Some(ty) = ty else {
+                assert_eq!(chain.len(), 0);
+            };
+            };
             if chain.state == ChainState::Invalidated {
                 assert_eq!(chain.local_copies.len(), 0);
  local time: Fri Aug 18 03:14:33 UTC 2023
  network time: Fri, 18 Aug 2023 03:14:33 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

pnkfelix · 2023-09-13T18:42:07Z

(I have decided that time would be better spent trying to fix the problem in a more general manner rather than trying to apply band-aids like what this PR was proposing.)

rustbot assigned cjgillot Mar 1, 2023

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-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 1, 2023

pnkfelix changed the title ~~mir-opt for generators/futures: copy propagate upvar into locals~~ [WIP] mir-opt for generators/futures: copy propagate upvar into locals Mar 1, 2023

Nilstrieb reviewed Mar 1, 2023

View reviewed changes

tests/ui/async-await/future-sizes/async-awaiting-fut.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot 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 Mar 1, 2023

cjgillot reviewed Mar 1, 2023

View reviewed changes

Swatinem mentioned this pull request Mar 5, 2023

Moka get_with inflates future size by ~7x moka-rs/moka#212

Closed

csmoe reviewed Mar 7, 2023

View reviewed changes

pnkfelix mentioned this pull request Mar 8, 2023

async block occupies twice the size of its captures (causing explosion in size). #108906

Open

pnkfelix force-pushed the copy-prop-upvar-to-elim-local-for-62958 branch from 4f9e527 to 020d08b Compare March 21, 2023 20:11

saethlin mentioned this pull request Jul 25, 2023

Trivial wrapper Generators waste space relative to wrapped Generators #114064

Closed

pnkfelix added 3 commits August 17, 2023 11:45

refactoring: move replace_base utility function to root of rustc_mi…

6277dbb

…r_transform crate. incorporated review feedback: simplify replace_base via Place::project_deeper

pnkfelix added 3 commits August 17, 2023 23:10

Updated the future-sizes tests to opt-into the MIR transformations (r…

83e15f8

…egardless of their default settings) and then updated the numbers to reflect the improvements those transformations now yield.

Updated the print-type-sizes async.rs test to opt into the new MIR

6dbe78d

transformations added here (regardless of their default settings) and updated the numbers in the output to reflect the improvements those transformations yield.

pnkfelix force-pushed the copy-prop-upvar-to-elim-local-for-62958 branch from 020d08b to f4a1d4b Compare August 18, 2023 03:10

pnkfelix closed this Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pnkfelix commented Mar 1, 2023 •

edited

Loading

rustbot commented Mar 1, 2023

rustbot commented Mar 1, 2023

pnkfelix commented Mar 1, 2023

pnkfelix commented Mar 1, 2023

pnkfelix commented Mar 1, 2023

saethlin commented Mar 1, 2023

This comment has been minimized.

pnkfelix commented Mar 1, 2023 •

edited

Loading

pnkfelix commented Mar 1, 2023

pnkfelix commented Mar 1, 2023

cjgillot left a comment

cjgillot Mar 1, 2023

cjgillot Mar 1, 2023

cjgillot Mar 1, 2023

cjgillot Mar 1, 2023

pnkfelix Mar 1, 2023

cjgillot Mar 1, 2023

pnkfelix Mar 2, 2023 •

edited

Loading

cjgillot Mar 1, 2023

pnkfelix Mar 1, 2023

pnkfelix Mar 1, 2023 •

edited

Loading

saethlin Mar 1, 2023

pnkfelix Mar 2, 2023

pnkfelix Mar 2, 2023

saethlin commented Mar 1, 2023 •

edited

Loading

csmoe Mar 7, 2023 •

edited

Loading

pnkfelix Mar 21, 2023

pnkfelix Mar 21, 2023

pnkfelix commented Mar 8, 2023

bors commented Apr 29, 2023

rust-log-analyzer commented Aug 18, 2023

pnkfelix commented Sep 13, 2023

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

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

Conversation

pnkfelix commented Mar 1, 2023 • edited Loading

rustbot commented Mar 1, 2023

rustbot commented Mar 1, 2023

pnkfelix commented Mar 1, 2023

pnkfelix commented Mar 1, 2023

pnkfelix commented Mar 1, 2023

saethlin commented Mar 1, 2023

This comment has been minimized.

pnkfelix commented Mar 1, 2023 • edited Loading

pnkfelix commented Mar 1, 2023

pnkfelix commented Mar 1, 2023

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Mar 1, 2023 • edited Loading

csmoe Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Mar 8, 2023

bors commented Apr 29, 2023

rust-log-analyzer commented Aug 18, 2023

pnkfelix commented Sep 13, 2023

pnkfelix commented Mar 1, 2023 •

edited

Loading

pnkfelix commented Mar 1, 2023 •

edited

Loading

pnkfelix Mar 2, 2023 •

edited

Loading

pnkfelix Mar 1, 2023 •

edited

Loading

saethlin commented Mar 1, 2023 •

edited

Loading

csmoe Mar 7, 2023 •

edited

Loading