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

Simplify some code that depend on Deref #97077

Merged
merged 4 commits into from
Jul 26, 2022
Merged

Simplify some code that depend on Deref #97077

merged 4 commits into from
Jul 26, 2022

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented May 16, 2022

Now that we can assume #97025 works, it's safe to expect Deref is always in the first place of projections. With this, I was able to simplify some code that depended on Deref's place in projections. When we are able to move Derefer before ElaborateDrops successfully we will be able to optimize more places.

r? @oli-obk

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 16, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented May 22, 2022

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

@oli-obk oli-obk added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jun 2, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jun 2, 2022

Blocked on #95576

@bors
Copy link
Contributor

bors commented Jun 21, 2022

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

@pnkfelix
Copy link
Member

@rustbot label: -S-blocked +S-waiting-on-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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 21, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2022

📌 Commit 9f00d83 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 25, 2022
@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 25, 2022
@JohnTitor JohnTitor mentioned this pull request Jul 25, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
@bors
Auto merge of rust-lang#99735 - JohnTitor:rollup-d93jyr2, r=JohnTitor
a867059 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#92390 (Constify a few `(Partial)Ord` impls)
 - rust-lang#97077 (Simplify some code that depend on Deref)
 - rust-lang#98710 (correct the output of a `capacity` method example)
 - rust-lang#99084 (clarify how write_bytes can lead to UB due to invalid values)
 - rust-lang#99178 (Lighten up const_prop_lint, reusing const_prop)
 - rust-lang#99673 (don't ICE on invalid dyn calls)
 - rust-lang#99703 (Expose size_hint() for TokenStream's iterator)
 - rust-lang#99709 (`Inherited` always has `TypeckResults` available)
 - rust-lang#99713 (Fix sidebar background)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2989275 into rust-lang:master Jul 26, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 26, 2022
@ouz-a ouz-a deleted the Optimize-backend branch November 13, 2022 09:55
RalfJung
RalfJung reviewed Aug 3, 2023
View reviewed changes
compiler/rustc_middle/src/mir/mod.rs
@@ -1461,6 +1461,14 @@ impl<'tcx> Place<'tcx> {
self.projection.iter().any(|elem| elem.is_indirect())
}

/// If MirPhase >= Derefered and if projection contains Deref,
/// It's guaranteed to be in the first place
pub fn has_deref(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super confusing, we now have is_indirect and has_deref and they are basically checking the same thing except one assumes we are in a certain MIR phase and the other doesn't. These doc comments of both methods should explain when to use one vs the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this function, I don't quite remember the details why we needed to check if Deref was in first place or not but I remember it making quite big difference.

RalfJung
RalfJung reviewed Aug 3, 2023
View reviewed changes
compiler/rustc_middle/src/mir/mod.rs
/// If MirPhase >= Derefered and if projection contains Deref,
/// It's guaranteed to be in the first place
pub fn has_deref(&self) -> bool {
self.projection.first() == Some(&PlaceElem::Deref)
Copy link
Member

@RalfJung RalfJung Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same debug assertion should also be added here -- right now it seems very easy to accidentally call has_deref before the "deref only in first projection" invariant has been established, and then we'll get the weirdest bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the debug assertion and bit better documentation here #114505

@RalfJung RalfJung mentioned this pull request Aug 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@oli-obk oli-obk

Labels
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.
8 participants
@ouz-a @rust-log-analyzer @bors @oli-obk @pnkfelix @rustbot @RalfJung @rust-highfive