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.

Sign up for GitHub

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

Jump to bottom

Change scope of temporaries in match guards #88678

Merged
merged 1 commit into from
Sep 6, 2021
Merged

Change scope of temporaries in match guards #88678

merged 1 commit into from
Sep 6, 2021

Conversation

matthewjasper
Copy link
Contributor

Each pattern in a match arm has its own copy of the match guard in MIR, with its own temporary, so it has to be dropped before the the guards are joined to the single copy of the arm. This PR changes then_else_break to allow it to put the temporary in the innermost scope possible. This change isn't done for if expressions because that affects a large number of mir-opt tests and could more significantly affect performance.

closes #88649

r? @oli-obk

@matthewjasper
Change scope of temporaries in match guards
ad7f109 
Each pattern in a match arm has its own copy of the match guard in MIR,
with its own temporary, so it has to be dropped before the the guards
are joined to the single copy of the arm.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Sep 6, 2021 •
edited
Loading

@bors r+ rollup=never p=1 need to get it in before beta cutoff

@bors
Copy link
Contributor

bors commented Sep 6, 2021

📌 Commit ad7f109 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2021
@bors
Copy link
Contributor

bors commented Sep 6, 2021

⌛ Testing commit ad7f109 with merge 1c858ba...

@bors
Copy link
Contributor

bors commented Sep 6, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 1c858ba to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2021
@bors bors merged commit 1c858ba into rust-lang:master Sep 6, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 6, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1c858ba): comparison url.

Summary: This change led to very small relevant improvements 🎉 in compiler performance.

  • Very small improvement in instruction counts (up to -0.3% on incr-full builds of clap-rs)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2021
@rylev
Copy link
Member

rylev commented Sep 6, 2021

The waiting on review label was added accidentally, since we're now posting perf results after merges happen. I'll remove the label.

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

Also, this change is very small and is not considered "definitely relevant". In the future, such a change would be registered as "not a significant performance impact".

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 6, 2021
@RalfJung RalfJung mentioned this pull request Sep 6, 2021
Merged
@ghost ghost mentioned this pull request Sep 6, 2021
Merged
@camelid
Copy link
Member

camelid commented Sep 6, 2021 •
edited
Loading

Beta-nominating for backport to 1.56 because it seems that #88572 landed in 1.56, which is now beta, but this fix landed in 1.57.

See also the discussion on Zulip.

@camelid camelid added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 6, 2021
@matthewjasper matthewjasper deleted the if-boolean-scoping branch September 6, 2021 17:56
jackh726 added a commit to jackh726/rust that referenced this pull request Sep 8, 2021
@jackh726
Rollup merge of rust-lang#88691 - hyd-dev:88649, r=Mark-Simulacrum
f69ccb1 
Add a regression test for rust-lang#88649

I noticed that rust-lang#88649 does not have a regression test, so I add one in this PR.

The test fails with this without rust-lang#88678:
```
error[E0080]: evaluation of constant value failed
  --> /checkout/src/test/ui/consts/issue-88649.rs:13:52
   |
LL |             Foo::Variant1(x) | Foo::Variant2(x) if x => {}
   |                                                    ^ StorageLive on a local that was already live

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.
```
@matthewjasper matthewjasper added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 9, 2021
@pnkfelix
Copy link
Member

Discussion on the compiler team meeting led to some questions: Is the scope of #88649 restricted to just match, due to its use of guards on match arms? Or can that issue manifest itself with other control flow constructs, such as if let or even if?

@pnkfelix
Copy link
Member

beta backport approved. See also PR #88691

@cuviper cuviper added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 16, 2021
@cuviper
Copy link
Member

cuviper commented Sep 16, 2021

(Don't forget the label -- I only noticed this because of the comment on the other PR)

@cuviper cuviper mentioned this pull request Sep 16, 2021
Merged
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 16, 2021
@cuviper cuviper removed this from the 1.57.0 milestone Sep 16, 2021
@cuviper cuviper added this to the 1.56.0 milestone Sep 16, 2021
@matthewjasper
Copy link
Contributor Author

The original bug only affects match guards. The comment in the PR refers to making a similar change for if expressions which would not fix any bug but would have other potential impacts.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2021
@bors
Auto merge of rust-lang#88946 - cuviper:beta-next, r=Mark-Simulacrum
deef866 
[beta] backports

- rustdoc: Fix ICE with `doc(hidden)` on tuple variant fields rust-lang#88639
- Fix 2021 `dyn` suggestion that used code as label rust-lang#88657
- Workaround blink/chromium grid layout limitation of 1000 rows rust-lang#88776
- Change scope of temporaries in match guards rust-lang#88678
- Add a regression test for rust-lang#88649 rust-lang#88691
- Revert anon union parsing rust-lang#88775
- Disable validate_maintainers. rust-lang#88977

Also drop stage0 rustfmt, because that's only supposed to be used on master.

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.

StorageLive is added on a local that was already live
10 participants
@matthewjasper @oli-obk @bors @rust-timer @rylev @camelid @pnkfelix @cuviper @rust-highfive @rustbot