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

Clean up MIR drop generation #61872

Merged
merged 6 commits into from Jun 26, 2019

Conversation

Projects
None yet
9 participants
@matthewjasper
Copy link
Contributor

commented Jun 15, 2019

  • Don't assign twice to the destination of a while loop containing a break expression
  • Use as_temp to evaluate statement expression
  • Avoid consecutive StorageLives for the condition of a while loop
  • Unify return, break and continue handling, and move it to scopes.rs
  • Make some of the scopes.rs internals private
  • Don't use Places that are always Locals in MIR drop generation

Closes #42371
Closes #61579
Closes #61731
Closes #61834
Closes #61910
Closes #62115

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2019

r? @eddyb

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

@matthewjasper matthewjasper changed the title Refactor MIR drop generation Clean up MIR drop generation Jun 15, 2019

@@ -179,20 +179,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// conduct the test, if necessary
let body_block;
if let Some(cond_expr) = opt_cond_expr {

This comment has been minimized.

Copy link
@Centril

Centril Jun 15, 2019

Member

N.B. As part of implementing while a && let b = c { ... } I'll make a PR to remove hir::ExprKind::While and then hair::ExprKind::Loop will presumably need to drop it's condition field and so the else branch would be floated out and the if branch would be removed... I hope the changes here do not cause problems for this...?

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Jun 15, 2019

Author Contributor

That should be fine, this is only a problem because while loops are special cased. For any sensible HIR lowering this won't be a problem. The temporary will probably end up storage-live for the whole loop body, but that isn't observable. The following two functions generate almost identical optimized MIR after this PR, for example.

fn while_loop(c: bool) {
    while get_bool(c) {
        if get_bool(c) {
            break;
        }
    }
}

// What the above should probably expand to
fn exp_while_loop(c: bool) {
    loop {
        match get_bool(c) {
            true => {
                {if get_bool(c) {
                    break;
                }}
                continue;
            }
            _ => {}
        }
        break;
    }
}

This comment has been minimized.

Copy link
@Centril

Centril Jun 15, 2019

Member

Cool.

For any sensible HIR lowering this won't be a problem.

Specifically, the HIR lowering should likely be:

'label: while $cond $block

==>

'label: loop {
    match DropTemps($cond) {
        true => $block,
        _ => break,
    }
}

(is there a particular reason you are using continue; and an empty block?)

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Jun 15, 2019

Author Contributor

It's slightly closer to what the RFC specified. What you're suggesting is probably better.

This comment has been minimized.

Copy link
@Centril

Centril Jun 20, 2019

Member

This should be unnecessary now with #61988.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

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

@matthewjasper matthewjasper force-pushed the matthewjasper:refactor-mir-drop-gen branch from 5ae55a3 to 2c93002 Jun 16, 2019

@matthewjasper matthewjasper force-pushed the matthewjasper:refactor-mir-drop-gen branch from 2c93002 to 13218c2 Jun 17, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

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

@eddyb

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@rust-highfive rust-highfive assigned pnkfelix and unassigned eddyb Jun 18, 2019

@cramertj cramertj referenced this pull request Jun 18, 2019

Merged

Fix HIR visit order #61572

anp added a commit to anp/moxie that referenced this pull request Jun 20, 2019

Roll toolchain back to avoid a known ICE.
Should be able to roll forward when rust-lang/rust#61872 is in a nightly.

@anp anp referenced this pull request Jun 21, 2019

Open

Enable clippy in CI #7

@matthewjasper matthewjasper force-pushed the matthewjasper:refactor-mir-drop-gen branch from 13218c2 to 07e5297 Jun 22, 2019

azastrael added a commit to aktoro-rs/aktoro that referenced this pull request Jun 22, 2019

@azastrael azastrael referenced this pull request Jun 24, 2019

Closed

TCP/UDP streams/sockets #12

4 of 4 tasks complete
@nikomatsakis
Copy link
Contributor

left a comment

This looks very good. I left a few questions.

Show resolved Hide resolved src/librustc_mir/build/expr/stmt.rs
Show resolved Hide resolved src/librustc_mir/build/matches/mod.rs
Show resolved Hide resolved src/librustc_mir/build/expr/stmt.rs
Show resolved Hide resolved src/test/mir-opt/while-storage.rs
Show resolved Hide resolved src/librustc_mir/build/expr/as_rvalue.rs
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

📌 Commit 07e5297 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

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

@matthewjasper matthewjasper force-pushed the matthewjasper:refactor-mir-drop-gen branch from 07e5297 to 3131427 Jun 25, 2019

@cramertj

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

📌 Commit 3131427 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

⌛️ Testing commit 3131427 with merge d3e2cec...

bors added a commit that referenced this pull request Jun 26, 2019

Auto merge of #61872 - matthewjasper:refactor-mir-drop-gen, r=nikomat…
…sakis

Clean up MIR drop generation

* Don't assign twice to the destination of a `while` loop containing a `break` expression
* Use `as_temp` to evaluate statement expression
* Avoid consecutive `StorageLive`s for the condition of a `while` loop
* Unify `return`, `break` and `continue` handling, and move it to `scopes.rs`
* Make some of the `scopes.rs` internals private
* Don't use `Place`s that are always `Local`s in MIR drop generation

Closes #42371
Closes #61579
Closes #61731
Closes #61834
Closes #61910
Closes #62115
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: nikomatsakis
Pushing d3e2cec to master...

@bors bors added the merged-by-bors label Jun 26, 2019

@bors bors merged commit 3131427 into rust-lang:master Jun 26, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
pr Build #20190625.57 succeeded
Details

cramertj added a commit to cramertj/rust that referenced this pull request Jun 26, 2019

@theduke theduke referenced this pull request Jun 26, 2019

Open

[Stabilization] async/await MVP #62149

2 of 4 tasks complete
@mati865

This comment has been minimized.

@matthewjasper matthewjasper deleted the matthewjasper:refactor-mir-drop-gen branch Jul 7, 2019

@matthewjasper

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

The ctfe-stress benchmark is now generating more MIR, so I'm not surprised that it's slower. I can't really work out what's going on with unicode_normalization, since the generated MIR is unchanged for the large functions and constants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.