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

Move promotion into its own pass #65949

Merged
merged 5 commits into from
Nov 9, 2019
Merged

Move promotion into its own pass #65949

merged 5 commits into from
Nov 9, 2019
+186 −197

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Oct 29, 2019 •
edited
Loading

edited

This adds a PromoteTemps pass, which runs after the old QualifyAndPromoteConsts pass, that only does promotion (no const-checking). Everything related to promotion has been removed from QualifyAndPromoteConstants: it no longer even visits the body of a non-const fn.

As a result we no longer need to keep the BitSet of promotable locals that was returned by mir_const_qualif. Rvalue-static promotion in a const is now done in promote_consts, and it operates on a set of Candidates instead. This will allow me–in a later PR–to create promoted MIR fragments for consts when necessary, which could resolve some shortcomings of the current approach (removing StorageDead).

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2019
@ecstatic-morse ecstatic-morse changed the title Move promotion into its own pass [WIP] Move promotion into its own pass Oct 29, 2019
@ecstatic-morse ecstatic-morse changed the title [WIP] Move promotion into its own pass [DO NOT MERGE] Move promotion into its own pass Oct 29, 2019
@ecstatic-morse ecstatic-morse mentioned this pull request Oct 29, 2019
Closed
4 tasks
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@eddyb
Copy link
Member

eddyb commented Oct 30, 2019

I would prefer to remove the minimum possible (maybe just number two?) to avoid needless work

Is popping bubble wrap needless work 😁? I can do some of it if you want.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Oct 30, 2019 •
edited
Loading

Is popping bubble wrap needless work grin? I can do some of it if you want.

I actually laughed out loud at this.

Sorry, my post reads as a bit petulant to me today. The last time I did a major change to qualify_consts it didn't turn out very well (#63860). I was secretly hoping the next major change to it would be made with rm 😄.

I didn't want to waste time removing stuff that you didn't really want removed, so if you tell me you want all dead code (including the full checklist) gone I can do that. Alternatively, if it really is like popping bubble wrap for you, have at it.

@eddyb
Copy link
Member

eddyb commented Oct 31, 2019

Just remembered I saw somewhere someone point out that beta branches on November 5th.

I'd suggest we remove the old promotion logic from qualify_consts just after that, to make sure the comparison trickles into beta and stable.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Oct 31, 2019 •
edited
Loading

I'd suggest we remove the old promotion logic from qualify_consts just after that, to make sure the comparison trickles into beta and stable.

Agreed. This would be ideal.

@ecstatic-morse ecstatic-morse mentioned this pull request Nov 5, 2019
Merged
@bors
Copy link
Contributor

bors commented Nov 6, 2019

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

@ecstatic-morse ecstatic-morse changed the title [DO NOT MERGE] Move promotion into its own pass Move promotion into its own pass Nov 6, 2019
@ecstatic-morse
Stop returning promotables from mir_const_qualif
3783ef6
@ecstatic-morse
Add a PromoteTemps pass
170272b 
`remove_storage_dead_and_drop` has been copied to `promote_temps` and
now operates on an array of `Candidate`s instead of a bitset.
@ecstatic-morse
Use new PromoteTemps for promotion
b316384
@ecstatic-morse
Bless tests now that we do promotion if min_const_fn fails
b13c2c3 
We bailed out of `QualifyAndPromoteConsts` immediately if the
`min_const_fn` checks failed, which sometimes resulted in additional,
spurious errors since promotion was skipped.

We now do promotion in a completely separate pass, so this is no longer
an issue.
@ecstatic-morse
Clean up dead code in qualify_consts
a3b0369 
We don't do promotion here anymore, so `Checker` will never even visit
the body of a non-const `fn`.
@ecstatic-morse
Copy link
Contributor Author

@eddyb This is ready for review now that #66066 has been merged.

eddyb
eddyb reviewed Nov 9, 2019
View reviewed changes
src/test/ui/consts/min_const_fn/min_const_fn.stderr
| ^--
| ||
| |temporary value created here
| returns a reference to data owned by the current function
Copy link
Member

Choose a reason for hiding this comment

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

What was this bug, looks confusing, maybe related to min_const_fn checks? cc @oli-obk

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, there's a commit that explains this!

@eddyb
Copy link
Member

eddyb commented Nov 9, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2019

📌 Commit a3b0369 has been approved by eddyb

@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 Nov 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 9, 2019
@Centril
Rollup merge of rust-lang#65949 - ecstatic-morse:promote-only-pass, r…
aaceede 
…=eddyb

Move promotion into its own pass

**edited**

This adds a `PromoteTemps` pass, which runs after the old `QualifyAndPromoteConsts` pass, that *only* does promotion (no const-checking). Everything related to promotion has been removed from `QualifyAndPromoteConstants`: it no longer even visits the body of a non-const `fn`.

As a result we no longer need to keep the `BitSet` of promotable locals that was returned by `mir_const_qualif`. Rvalue-static promotion in a `const` is now done in `promote_consts`, and it operates on a set of `Candidate`s instead. This will allow me–in a later PR–to create promoted MIR fragments for `const`s when necessary, which could resolve some shortcomings of the current approach (removing `StorageDead`).

r? @eddyb
@Centril Centril mentioned this pull request Nov 9, 2019
Merged
bors added a commit that referenced this pull request Nov 9, 2019
@bors
Auto merge of #66242 - Centril:rollup-h73ztr1, r=Centril
5a50275 
Rollup of 6 pull requests

Successful merges:

 - #65949 (Move promotion into its own pass)
 - #65994 (Point at where clauses where the associated item was restricted)
 - #66050 (Fix C aggregate-passing ABI on powerpc)
 - #66134 (Point at formatting descriptor string when it is invalid)
 - #66172 (Stabilize @file command line arguments)
 - #66226 (add link to unstable book for asm! macro)

Failed merges:

r? @ghost
@bors bors merged commit a3b0369 into rust-lang:master Nov 9, 2019
@ecstatic-morse ecstatic-morse deleted the promote-only-pass branch October 6, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centril Centril Centril left review comments

@eddyb eddyb eddyb approved these changes

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ecstatic-morse @rust-highfive @eddyb @bors @Centril