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

Avoid duplicating StorageLive in let-else #101894

Merged
merged 3 commits into from Sep 19, 2022
Merged

Avoid duplicating StorageLive in let-else #101894

merged 3 commits into from Sep 19, 2022

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Sep 16, 2022 •
edited

cc @est31

Fix #101867
Fix #101932

#101410 introduced directives to activate storages of bindings in let-else earlier. However, since it is using the machinery of match and friends for pattern matching and binding, those storages are activated for the second time. This PR adjusts this behavior and avoid the duplicated activation for let-else statements.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 16, 2022
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2022
@dingxiangfei2009
Copy link
Contributor Author

./x.py test src/tools/miri is now passing again after this change. You may try it out.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 16, 2022

Please also add a mir-opt test. #101410 (comment) seems like a good candidate as it is very minimal.

@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 16, 2022
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Sep 16, 2022 •
edited

Preemptively beta-nominating for backport to 1.65 (into which #101410 landed), since it seems likely this would otherwise only make 1.66. (If we reject this we'll likely want to post a revert for #101410, but this was an easier first step to track the need).

@dingxiangfei2009
Copy link
Contributor Author

@oli-obk Thank you! I have added that test mentioned in the #101410 thread. I have tentatively selected mir_map which is hopefully very close to the original MIR generated. Please advise better choices. 🙏

@est31 est31 mentioned this pull request Sep 17, 2022
Closed
@est31
Copy link
Member

est31 commented Sep 17, 2022

@dingxiangfei2009 now that #93628 has been merged, can you rebase this PR? Most importantly, the test files will need #![cfg_attr(bootstrap, feature(let_else))], instead of #![feature(let_else)].

Also, would it be possible to add the example from #101932 as a run-pass regression test? So add // run-pass to it as well as // issue #101932, and put it e.g. into src/test/ui/let-else/const-fn.rs.

@dingxiangfei2009 dingxiangfei2009 force-pushed the let-else-avoid-duplicate-storage-live branch from a7d87c9 to eb36f5e Compare September 18, 2022 04:18
@dingxiangfei2009
Copy link
Contributor Author

@est31 branch rebased with the test case from #101932

cc @oli-obk for the relevant changes.

@RalfJung
Copy link
Member

This issue is completely blocking Miri development, would be good to get the fix landed ASAP. :)

@oli-obk
Copy link
Contributor

oli-obk commented Sep 19, 2022

@bors r+ p=1 (unblocks miri)

@bors
Copy link
Contributor

bors commented Sep 19, 2022

📌 Commit eb36f5e has been approved by oli-obk

It is now in the queue for this repository.

@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 19, 2022
@RalfJung
Copy link
Member

@bors p=10

@bors
Copy link
Contributor

bors commented Sep 19, 2022

⌛ Testing commit eb36f5e with merge 2019147...

@oli-obk oli-obk mentioned this pull request Sep 19, 2022
Merged
@bors
Copy link
Contributor

bors commented Sep 19, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2022
@bors bors merged commit 2019147 into rust-lang:master Sep 19, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 19, 2022
@bors bors mentioned this pull request Sep 19, 2022
Open
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2019147): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.6% [2.2%, 3.3%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Footnotes

  1. the arithmetic mean of the percent change ↩

  2. number of relevant changes ↩

@dingxiangfei2009 dingxiangfei2009 deleted the let-else-avoid-duplicate-storage-live branch September 20, 2022 06:46
@apiraino
Copy link
Contributor

apiraino commented Sep 22, 2022 •
edited

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 22, 2022
@RalfJung
Copy link
Member

Judging from this the release team will do the beta backports so I consider this done from my end now. :)

@cjgillot cjgillot mentioned this pull request Sep 25, 2022
Merged
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Sep 26, 2022
@fee1-dead
Rollup merge of rust-lang#102257 - cjgillot:let-else-lint, r=dingxian…
c2308a5 
…gfei2009

Fix lint scoping for let-else.

The scoping for let-else is inconsistent with HIR nesting.  This creates cases, in `ui/let-else/let-else-allow-unused.rs` for instance, where an `allow` lint attribute does not apply to the bindings created by `let-else`.

This PR is an attempt to correct this.

As there is no lint that currently relies on this, the test for this behaviour is rust-lang#101500.

cc `@dingxiangfei2009` as you filed rust-lang#101894
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 26, 2022
@bors
Auto merge of rust-lang#102257 - cjgillot:let-else-lint, r=dingxiangf…
1d1f142 
…ei2009

Fix lint scoping for let-else.

The scoping for let-else is inconsistent with HIR nesting.  This creates cases, in `ui/let-else/let-else-allow-unused.rs` for instance, where an `allow` lint attribute does not apply to the bindings created by `let-else`.

This PR is an attempt to correct this.

As there is no lint that currently relies on this, the test for this behaviour is rust-lang#101500.

cc `@dingxiangfei2009` as you filed rust-lang#101894
@cuviper cuviper mentioned this pull request Oct 4, 2022
Merged
@cuviper cuviper modified the milestones: 1.66.0, 1.65.0 Oct 4, 2022
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 4, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2022
@bors
Auto merge of rust-lang#102676 - cuviper:beta-next, r=cuviper
da7ffa2 
[beta] backports

* Avoid duplicating StorageLive in let-else rust-lang#101894
* Re-add HRTB implied static bug note rust-lang#101924
* Revert "Copy stage0 binaries into stage0-sysroot" rust-lang#101942
* implied_bounds: deal with inference vars rust-lang#102016
* fix ConstProp handling of written_only_inside_own_block_locals rust-lang#102045
* Fix wrongly refactored Lift impl rust-lang#102088
* Fix a typo “pararmeter” in error message rust-lang#102119
* Deny associated type bindings within associated type bindings rust-lang#102338
* Continue migration of CSS themes rust-lang#101934
* Fix search result colors rust-lang#102369
* Fix unwind drop glue for if-then scopes rust-lang#102394
* Revert "Use getentropy when possible on all Apple platforms" rust-lang#102693
* Fix associated type bindings with anon const in GAT position rust-lang#102336
* Revert perf-regression 101620 rust-lang#102064
* `EscapeAscii` is not an `ExactSizeIterator` rust-lang#99880
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk Awaiting requested review from oli-obk

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.65.0
Development

Successfully merging this pull request may close these issues.

StorageLive already live in const let else regression miri no longer builds after rust-lang/rust#101410