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

[mir-opt] Small ConstProp cleanup #71911

Merged
merged 2 commits into from
Jun 21, 2020
Merged

[mir-opt] Small ConstProp cleanup #71911

merged 2 commits into from
Jun 21, 2020

Conversation

wesleywiser
Copy link
Member

No description provided.

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 5, 2020
@rust-highfive

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 5, 2020

⌛ Trying commit 689422b959b40de1da8c0a37ea4dcd63a4bb8451 with merge 8abd3d455988e5bf83282b3abdd7c23a1362bc96...

@pietroalbini
Copy link
Member

Note: bors will probably mark the try build as failed, since the try build was started before we started double-gating on both Azure Pipelines and GitHub Actions.

@wesleywiser
Copy link
Member Author

bors looks stuck in the "pending" state for GHA probably for the reasons @pietroalbini mentioned. I'll try it again to see if that fixes it:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 5, 2020

⌛ Trying commit 689422b959b40de1da8c0a37ea4dcd63a4bb8451 with merge 30692efbe3de0e909467c2c01e14685f76efb576...

@bors
Copy link
Contributor

bors commented May 5, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 30692efbe3de0e909467c2c01e14685f76efb576 (30692efbe3de0e909467c2c01e14685f76efb576)

@rust-timer
Copy link
Collaborator

Queued 30692efbe3de0e909467c2c01e14685f76efb576 with parent f8d394e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 30692efbe3de0e909467c2c01e14685f76efb576, comparison URL.

@wesleywiser wesleywiser mentioned this pull request May 12, 2020
Merged
@crlf0710 crlf0710 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 May 15, 2020
@crlf0710
Copy link
Member

There's a merge conflict now. Needs a rebase here.

@joelpalmer joelpalmer 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 26, 2020
@Elinvynia Elinvynia 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2020
@oli-obk oli-obk added the A-mir-opt Area: MIR optimizations label Jun 6, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 6, 2020

r? @oli-obk

r=me after a rebase. The perf wins are real and the perf losses are cgu related

@bors
Copy link
Contributor

bors commented Jun 15, 2020

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

@wesleywiser
[const-prop] Remove ConstPropMode::NoPropagation
c06876c 
This mode is unnecessary because it's always ok to evaluate the
right-hand side of assignments even if the left-hand side should not
have reads propagated.
@wesleywiser
Add EMIR_MIR_FOR_EACH_BIT_WIDTH to failing test
2f49d55
@wesleywiser
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jun 16, 2020

📌 Commit 2f49d55 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 16, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
@Manishearth
Rollup merge of rust-lang#71911 - wesleywiser:const_prop_small_cleanu…
3d6cb74 
…ps, r=oli-obk

[mir-opt] Small ConstProp cleanup
@Manishearth Manishearth mentioned this pull request Jun 16, 2020
Closed
@bors
Copy link
Contributor

bors commented Jun 20, 2020

⌛ Testing commit 2f49d55 with merge 2150cf111bb697fffd5bd8115469957d1e1e24d6...

@bors
Copy link
Contributor

bors commented Jun 20, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 20, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 20, 2020

@bors retry

@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 Jun 20, 2020
@Manishearth
Copy link
Member

@bors p=1

letting rollup=never PRs flush out

@bors
Copy link
Contributor

bors commented Jun 21, 2020

⌛ Testing commit 2f49d55 with merge 38bd83d...

@bors
Copy link
Contributor

bors commented Jun 21, 2020

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 38bd83d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2020
@bors bors merged commit 38bd83d into rust-lang:master Jun 21, 2020
@bors bors mentioned this pull request Jun 21, 2020
Merged
@RalfJung RalfJung mentioned this pull request Jun 22, 2020
Closed
@RalfJung
Copy link
Member

This PR introduced a miscompilation in mir-opt-level=3: #73609

@RalfJung
Copy link
Member

In fact it's not just opt-level 3, it's even the default opt-level 1. This introduced a stable-to-nightly regression.

RalfJung
RalfJung reviewed Jun 22, 2020
View reviewed changes
src/librustc_mir/transform/const_prop.rs
@@ -820,7 +818,7 @@ impl<'tcx> Visitor<'tcx> for CanConstProp {
| MutatingUse(MutatingUseContext::Borrow)
| MutatingUse(MutatingUseContext::AddressOf) => {
trace!("local {:?} can't be propagaged because it's used: {:?}", local, context);
self.can_const_prop[local] = ConstPropMode::NoPropagation;
Copy link
Member

Choose a reason for hiding this comment

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

@oli-obk so is this the diff that caused the bug? That these uses no longer prevent propagation? Instead of duplicating that use detection by handling Ref and AddrOf ourselves, would it make more sense to continue to use this info here (or at least do so additionally)?

@pnkfelix pnkfelix mentioned this pull request Jul 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

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

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@oli-obk oli-obk

Labels
A-mir-opt Area: MIR optimizations 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
No milestone
Development

Successfully merging this pull request may close these issues.