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

Do some simple constant propagation in the ConstProp pass #60597

Merged
merged 4 commits into from May 17, 2019
Merged

Do some simple constant propagation in the ConstProp pass #60597

merged 4 commits into from May 17, 2019

Conversation

wesleywiser
Copy link
Member

r? @oli-obk

I added a few test cases. Let me know if you think there should be more.

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

This comment has been minimized.

Sign in to view
oli-obk
oli-obk requested changes May 7, 2019
View reviewed changes
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/const_prop.rs Outdated Show resolved Hide resolved
@wesleywiser
Copy link
Member Author

Feedback resolved

@oli-obk

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
bors added a commit that referenced this pull request May 7, 2019
@bors
Auto merge of #60597 - wesleywiser:const_prop, r=<try>
7f68fd8 
Do some simple constant propagation in the ConstProp pass

r? @oli-obk

I added a few test cases. Let me know if you think there should be more.
@oli-obk

This comment has been minimized.

Sign in to view
@rust-timer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented May 7, 2019

r? @eddyb

This is a very conservative constant propagator that will only replace the rhs of assignments with constants. Any other avenues for constant propagation are left for future changes.

@rust-highfive rust-highfive assigned eddyb and unassigned oli-obk May 7, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@wesleywiser
Copy link
Member Author

@oli-obk This is gated on mir_opt_level >= 3 so a perf run will only measure the overhead of that check. Did you want to see the performance of running the propagation? I can push a temporary commit which un-gates the constant propagation.

@oli-obk
Copy link
Contributor

oli-obk commented May 7, 2019

Did you want to see the performance of running the propagation? I can push a temporary commit which un-gates the constant propagation.

It's unlikely that this would cause performance changes of the compiled binary. LLVM should take care of that just fine. I wanted to run perfbot to see whether the const propagation slows down compilation. I assumed our perf test suite runs with full MIR optimizations, but I should have checked. I'll do that now

@wesleywiser
Copy link
Member Author

mir-opt-level >= 2 results in breakage so I doubt it.

@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors bors 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 7, 2019
@oli-obk
Copy link
Contributor

oli-obk commented May 7, 2019

Right. So we can check the perf when we decrease the minimum level where the propagator is turned on or make O2 actually pass all tests

@wesleywiser
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented May 8, 2019

⌛ Trying commit 9453e59332b5aa41a95d0b3ae9c262e3661aac46 with merge 0a38017c5f0419a3bce64fccf7d926df2ce08476...

@rust-highfive

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 reviewed May 11, 2019
View reviewed changes
src/test/mir-opt/const_prop/slice_len.rs
// return;
// }
// END rustc.main.ConstProp.before.mir
// START rustc.main.ConstProp.after.mir
Copy link
Member

Choose a reason for hiding this comment

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

Mir after const prop doesn't seem to be different from before.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's correct. I just added this test to try to "lock in" more of the behavior. I have additional changes to improve the constant propagation code incoming but per @oli-obk's feedback, I wanted to keep this PR small.

@wesleywiser wesleywiser mentioned this pull request May 11, 2019
Merged
bors added a commit that referenced this pull request May 11, 2019
@bors
Auto merge of #60745 - wesleywiser:const_prop_into_terminators, r=<try>
7f66c41 
[WIP] Const prop into terminators

Rebased on top of #60597. New commits start at eea75dc.

Last commit is just for testing purposes and I'll remove it before we merge this.

r? @ghost
eddyb
eddyb reviewed May 13, 2019
View reviewed changes
src/librustc_mir/transform/const_prop.rs
ScalarMaybeUndef::Scalar(two)
) => {
let ty = &value.layout.ty.sty;
if let ty::Tuple(substs) = ty {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why this is needed, you should be able to create a ty::Const from any constant.

@wesleywiser
Copy link
Member Author

Pushed fixes

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2019

@bors r+

@bors
Copy link
Contributor

bors commented May 16, 2019

📌 Commit b17066d 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 May 16, 2019
@bors
Copy link
Contributor

bors commented May 16, 2019

⌛ Testing commit b17066d with merge 5b7ec0965850679fc5122e82693401e041c1157e...

@bors
Copy link
Contributor

bors commented May 16, 2019

💔 Test failed - status-appveyor

@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 May 16, 2019
@wesleywiser
Copy link
Member Author

warning: spurious network error (2 tries remaining): failed to get 200 response from https://static.crates.io/crates/png/png-0.8.0.crate, got 504
warning: spurious network error (1 tries remaining): failed to get 200 response from https://static.crates.io/crates/png/png-0.8.0.crate, got 504
error: failed to download from https://crates.io/api/v1/crates/png/0.8.0/download
Caused by:
failed to get 200 response from https://static.crates.io/crates/png/png-0.8.0.crate, got 504
thread 'main' panicked at 'tests failed for https://github.com/servo/webrender', src\tools\cargotest\main.rs:88:9
note: Run with RUST_BACKTRACE=1 environment variable to display a backtrace.

@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 May 16, 2019
Manishearth added a commit to Manishearth/rust that referenced this pull request May 16, 2019
@Manishearth
Rollup merge of rust-lang#60597 - wesleywiser:const_prop, r=oli-obk
e7f2794 
Do some simple constant propagation in the ConstProp pass

r? @oli-obk

I added a few test cases. Let me know if you think there should be more.
@Manishearth Manishearth mentioned this pull request May 16, 2019
Merged
bors added a commit that referenced this pull request May 16, 2019
@bors
Auto merge of #60888 - Manishearth:rollup-oihtoyq, r=Manishearth
4f53b5c 
Rollup of 5 pull requests

Successful merges:

 - #60207 (Outdent example, preserving nested fence)
 - #60278 (Document the `html_root_url` doc attribute value.)
 - #60597 (Do some simple constant propagation in the ConstProp pass)
 - #60837 (Update release notes for 1.35.0)
 - #60887 (Update clippy)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented May 17, 2019

⌛ Testing commit b17066d with merge 4f53b5c...

@bors bors merged commit b17066d into rust-lang:master May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@eddyb eddyb eddyb approved these changes

@oli-obk oli-obk oli-obk 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.

None yet
8 participants
@wesleywiser @rust-highfive @oli-obk @bors @rust-timer @Mark-Simulacrum @eddyb @bjorn3