Skip to content

Pull Derefer before ElaborateDrops #98145

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 13, 2022
Merged

Pull Derefer before ElaborateDrops #98145

merged 2 commits into from
Jul 13, 2022

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Jun 15, 2022

Follow up work to #97025 #96549 #96116 #95887 #95649

This moves Derefer before ElaborateDrops and creates a new Rvalue called VirtualRef that allows us to bypass many constraints for DerefTemp.

r? @oli-obk

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

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2022
@bors
Copy link
Collaborator

bors commented Jun 20, 2022

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

oli-obk
oli-obk requested changes Jun 20, 2022
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

# to view
@rust-log-analyzer

This comment has been minimized.

# to view
@rust-log-analyzer

This comment has been minimized.

# to view
@oli-obk
Copy link
Contributor

oli-obk commented Jun 21, 2022

@rustbot author

@rustbot 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 Jun 21, 2022
@ouz-a ouz-a force-pushed the some_branch branch 3 times, most recently from 9565104 to 2659f22 Compare June 21, 2022 12:45
@bors
Copy link
Collaborator

bors commented Jun 21, 2022

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

@bors
Copy link
Collaborator

bors commented Jun 22, 2022

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

@ouz-a ouz-a force-pushed the some_branch branch 4 times, most recently from 3243226 to aa91716 Compare June 22, 2022 19:37
@ouz-a ouz-a requested a review from oli-obk June 22, 2022 19:37
@oli-obk
Copy link
Contributor

oli-obk commented Jun 23, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 23, 2022

📌 Commit aa91716 has been approved by oli-obk

@bors
Copy link
Collaborator

bors commented Jun 23, 2022

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 23, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jul 12, 2022

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Jul 12, 2022

📌 Commit b4c3a2a 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2022
@bors
Copy link
Collaborator

bors commented Jul 13, 2022

⌛ Testing commit b4c3a2a with merge 42bd138...

@bors
Copy link
Collaborator

bors commented Jul 13, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 13, 2022
@bors bors merged commit 42bd138 into rust-lang:master Jul 13, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 13, 2022
@bors bors mentioned this pull request Jul 13, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (42bd138): comparison url.

Instruction count

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 0.5% 0.8% 13
Regressions 😿
(secondary)		 0.7% 1.1% 20
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 0.5% 0.8% 13

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 3.0% 3.1% 2
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -9.1% -9.1% 1
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -3.1% -3.1% 1
Improvements 🎉
(secondary)		 -2.9% -2.9% 1
All 😿🎉 (primary) -3.1% -3.1% 1

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added the perf-regression Performance regression. label Jul 13, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jul 14, 2022

hmm... looks like all mir queries got a few percent slower. I guess open an issue to track this regression to see if it goes away once we get rid of the new Rvalue

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
@bors
Auto merge of rust-lang#98145 - ouz-a:some_branch, r=oli-obk
4e3a7bf 
Pull Derefer before ElaborateDrops

_Follow up work to rust-lang#97025 rust-lang#96549 rust-lang#96116 rust-lang#95887 #95649_

This moves `Derefer` before `ElaborateDrops` and creates a new `Rvalue` called `VirtualRef` that allows us to bypass many constraints for `DerefTemp`.

r? `@oli-obk`
@rylev
Copy link
Member

rylev commented Jul 19, 2022

@oli-obk @ouz-a can you open an issue to track the regression here? I'm nervous that this will just get lost.

@Dylan-DPC Dylan-DPC mentioned this pull request Jul 19, 2022
Open
@Dylan-DPC
Copy link
Member

@oli-obk @ouz-a can you open an issue to track the regression here? I'm nervous that this will just get lost.

@rylev i opened #99473 for this

@ouz-a
Copy link
Contributor Author

ouz-a commented Jul 19, 2022

@oli-obk @ouz-a can you open an issue to track the regression here? I'm nervous that this will just get lost.

I was planning to open a PR without the new Rvalue

@ouz-a ouz-a mentioned this pull request Jul 20, 2022
Closed
@celinval celinval mentioned this pull request Jul 21, 2022
Merged
4 tasks
@ouz-a ouz-a mentioned this pull request Jul 24, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 29, 2022
@bors
Auto merge of rust-lang#99667 - ouz-a:some_branch, r=oli-obk
7dfdd64 
Optimize `UnDerefer`

Addresses the performance [issues](rust-lang#98145 (comment)) faced here.

r? `@oli-obk`
@beepster4096 beepster4096 mentioned this pull request Jun 21, 2023
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 30, 2023
@Dylan-DPC
Rollup merge of rust-lang#112882 - DrMeepster:new_un_derefer, r=oli-obk
b2ce846 
Rewrite `UnDerefer`

Currently, `UnDerefer` is used by drop elaboration to undo the effects of the `Derefer` pass. However, it just recreates the original places with derefs in the middle of the projection. Because `ProjectionElem::Deref` is intended to be removed completely in the future, this will not work forever.

This PR introduces a `deref_chain` method that returns the places behind `DerefTemp` locals in a place and rewrites the move path code to use this. In the process, `UnDerefer` was merged into `MovePathLookup`. Now that move paths use the same places as in the MIR, the other uses of `UnDerefer` no longer require it.

See rust-lang#98145
cc `@ouz-a`
r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 3, 2023
@bors
Auto merge of rust-lang#112882 - DrMeepster:new_un_derefer, r=oli-obk
d5a7424 
Rewrite `UnDerefer`

Currently, `UnDerefer` is used by drop elaboration to undo the effects of the `Derefer` pass. However, it just recreates the original places with derefs in the middle of the projection. Because `ProjectionElem::Deref` is intended to be removed completely in the future, this will not work forever.

This PR introduces a `deref_chain` method that returns the places behind `DerefTemp` locals in a place and rewrites the move path code to use this. In the process, `UnDerefer` was merged into `MovePathLookup`. Now that move paths use the same places as in the MIR, the other uses of `UnDerefer` no longer require it.

See rust-lang#98145
cc `@ouz-a`
r? `@oli-obk`
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@JakobDegen JakobDegen

@oli-obk oli-obk

@RalfJung RalfJung

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. 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.64.0
Development

Successfully merging this pull request may close these issues.