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.

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

Create promoted MIR fragments for const and statics #66642

Merged
merged 2 commits into from
Nov 28, 2019

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Nov 22, 2019

Resolves #65732.

The previous strategy of removing Drop and StorageDead for promoted locals only worked for rvalue lifetime extension and only if no loops were present. This PR applies the approach currently used for fn and const fns to const and statics.

This may have some performance impacts.

r? @eddyb

The previous test was incorrect. `const fn`s are *always* promotable
when inside a `const`, so it should pass. The error was caused by a bug
in `promote_consts`. I've added a failing test outside a `const`
alongside the existing one, which is now run-pass.
The previous strategy of removing `Drop` and `StorageDead` for promoted
locals only worked for rvalue lifetime extension. We now use the same
implementation for promotion across all kinds of items.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2019
@eddyb
Copy link
Member

eddyb commented Nov 27, 2019

r=me after running perf

@ecstatic-morse
Copy link
Contributor Author

@bors try
@rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 27, 2019

⌛ Trying commit f9ed219 with merge c17aff946ccec0822adaa7cc9c00a9e35e685a5e...

@bors
Copy link
Contributor

bors commented Nov 27, 2019

☀️ Try build successful - checks-azure
Build commit: c17aff946ccec0822adaa7cc9c00a9e35e685a5e (c17aff946ccec0822adaa7cc9c00a9e35e685a5e)

@rust-timer
Copy link
Collaborator

Queued c17aff946ccec0822adaa7cc9c00a9e35e685a5e with parent 04e69e4, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit c17aff946ccec0822adaa7cc9c00a9e35e685a5e, comparison URL.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Nov 27, 2019

This appears to actually be faster for ucd and html5ever. Presumably mutating MIR bodies in place is much slower than creating new ones?

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Nov 27, 2019

📌 Commit f9ed219 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 27, 2019
@bors

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 27, 2019

📌 Commit f9ed219 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Nov 28, 2019

⌛ Testing commit f9ed219 with merge bbb664a...

bors added a commit that referenced this pull request Nov 28, 2019
Create promoted MIR fragments for `const` and `static`s

Resolves #65732.

The previous strategy of removing `Drop` and `StorageDead` for promoted locals only worked for rvalue lifetime extension and only if no `loop`s were present. This PR applies the approach currently used for `fn` and `const fn`s to `const` and `statics`.

This may have some performance impacts.

r? @eddyb
@matthewjasper
Copy link
Contributor

matthewjasper commented Nov 28, 2019

This appears to actually be faster for ucd and html5ever. Presumably mutating MIR bodies in place is much slower than creating new ones?

I think it's more likely due to are borrowck (mostly) ignoring promoteds.

@bors
Copy link
Contributor

bors commented Nov 28, 2019

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing bbb664a to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

RFC 2203 not implemented inside a const block
7 participants