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

Rebase and fixup #80493: Remove MIR assignments to ZST types #83118

Merged
merged 6 commits into from
Mar 15, 2021
Merged

Rebase and fixup #80493: Remove MIR assignments to ZST types #83118

merged 6 commits into from
Mar 15, 2021

Conversation

erikdesjardins
Copy link
Contributor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tmiasko
Copy link
Contributor

tmiasko commented Mar 14, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 14, 2021
@bors
Copy link
Contributor

bors commented Mar 14, 2021

⌛ Trying commit 0f9af60 with merge 9bc206fe890926c6eeb208fec47dc5bc8e1dfe78...

@tmiasko
Copy link
Contributor

tmiasko commented Mar 14, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Mar 14, 2021

⌛ Trying commit 1b7b33e with merge 7f64e46687647913b6bfebdd755372360087954c...

@erikdesjardins
Copy link
Contributor Author

Hmm, does pushing mess up the try build? I thought it would be fine as long as I didn't force-push

@bors
Copy link
Contributor

bors commented Mar 14, 2021

☀️ Try build successful - checks-actions
Build commit: 7f64e46687647913b6bfebdd755372360087954c (7f64e46687647913b6bfebdd755372360087954c)

@rust-timer
Copy link
Collaborator

Queued 7f64e46687647913b6bfebdd755372360087954c with parent 9320b12, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (7f64e46687647913b6bfebdd755372360087954c): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 15, 2021
@erikdesjardins
remove unnecessary condition
47f8bac 
`_local` isn't visited in `_local = <rhs>` statements in the situation
we care about
@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2021

@bors r+

Thanks for reviving that PR!

@bors
Copy link
Contributor

bors commented Mar 15, 2021

📌 Commit 47f8bac 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 Mar 15, 2021
@bors
Copy link
Contributor

bors commented Mar 15, 2021

⌛ Testing commit 47f8bac with merge 7a7bbdb...

@bors
Copy link
Contributor

bors commented Mar 15, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 15, 2021
@bors bors merged commit 7a7bbdb into rust-lang:master Mar 15, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 15, 2021
@erikdesjardins erikdesjardins deleted the removezst branch March 15, 2021 17:13
@tmiasko
Copy link
Contributor

tmiasko commented Mar 15, 2021

That landed fast. I will just note that it might be interesting to explore removal of individual ZST assignments still, rather than locals, since this miss the opportunities that exists around code that is using ? with ZST types (? introduces var debug info and const propagation doesn't propagate into call terminators by default, so locals involved remain because they are used).

@erikdesjardins
Copy link
Contributor Author

That's what #80493 originally did before #80493 (comment): see https://github.com/rust-lang/rust/compare/22f94499cc9c397420c4cf8ba0bbcabf4018bae7

I'll open a PR.

@erikdesjardins erikdesjardins mentioned this pull request Mar 16, 2021
Merged
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Mar 16, 2021
@erikdesjardins
revert changes from rust-lang#83118 to mark zst return place as unused
affb081
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2021
@bors
Auto merge of rust-lang#83177 - erikdesjardins:zstassign, r=oli-obk
79e5814 
Remove assignments to ZST places instead of marking ZST return place as unused

partially reverts rust-lang#83118

requested by `@tmiasko` in rust-lang#83118 (comment)

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmiasko tmiasko tmiasko left review comments

Assignees

@oli-obk oli-obk

Labels
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.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.
9 participants
@erikdesjardins @rust-log-analyzer @tmiasko @rust-timer @bors @oli-obk @rust-highfive @rustbot @simonvandel