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

[mir-opt] Turn on the `ConstProp` pass by default #66074

Merged
merged 1 commit into from Nov 19, 2019

Conversation

@wesleywiser
Copy link
Member

wesleywiser commented Nov 4, 2019

perf.rlo shows that running the ConstProp pass results in
across-the-board wins regardless of debug or opt complilation mode. As a
result, we're turning it on to get the compile time benefits.

@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Nov 4, 2019

@bors try
@rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 4, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 4, 2019

⌛️ Trying commit 09d7ce8 with merge 98da694...

bors added a commit that referenced this pull request Nov 4, 2019
[experiment] Test ConstProp performance

Experiment to see performance implications for turning on ConstProp for scalars.

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 4, 2019

☀️ Try build successful - checks-azure
Build commit: 98da694 (98da694a4fda38435d86b0eb6f09d4f644ef83d4)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 4, 2019

Queued 98da694 with parent 0d5264a, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 4, 2019

Finished benchmarking try commit 98da694, comparison URL.

@oli-obk oli-obk added the T-compiler label Nov 6, 2019
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Nov 6, 2019

@rfcbot poll I think we should turn on constant propagation unconditionally (so in release mode and additionally in debug mode) since it reduces the work that llvm has to do quite a bit. Right now there is no real degradation of the debugging experience, since the worst that happens to users is that 2 + 2 shows up as 4 in the assembly and the debug info is still completely available, since it is attached to the temporary holding the 4 (or the result of 2 + 2 before this PR).

In the future we may const prop function calls to const fns, if that is deemed overkill for debug mode, we can always add an additional check that prevents function calls from being propagated in debug mode.

Constant propagation does not propagate over user defined variables, only expressions (so MIR temporaries) are const propagated. This may change in the future, but we can reopen the debug info degradation question then.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 6, 2019

Team member @oli-obk has asked teams: T-compiler, for consensus on:

I think we should turn on constant propagation unconditionally (so in release mode and additionally in debug mode) since it reduces the work that llvm has to do quite a bit. Right now there is no real degradation of the debugging experience, since the worst that happens to users is that 2 + 2 shows up as 4 in the assembly and the debug info is still completely available, since it is attached to the temporary holding the 4 (or the result of 2 + 2 before this PR).

@wesleywiser wesleywiser force-pushed the wesleywiser:test_run_const_prop branch from 09d7ce8 to db23021 Nov 6, 2019
@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 6, 2019

Note that post-#56231 you don't even need to care about debuginfo or "temp vs var", all you have to do is leave assignments around and run the pass to remove dead locals (which won't remove locals that have debuginfo attached).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Nov 7, 2019

@rfcbot reviewed

1 similar comment
@varkor

This comment has been minimized.

Copy link
Member

varkor commented Nov 8, 2019

@rfcbot reviewed

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Nov 8, 2019

I think we should turn on constant propagation unconditionally (so in release mode and additionally in debug mode)

Are the numbers above already with constant propagation turned on for debug builds too?

@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Nov 8, 2019

I think we should turn on constant propagation unconditionally (so in release mode and additionally in debug mode)

Are the numbers above already with constant propagation turned on for debug builds too?

@michaelwoerister Yes, the results are with constant propagation turned on for both debug and release builds.

bors added a commit that referenced this pull request Nov 8, 2019
…=<try>

[mir-opt] Handle return place in ConstProp and improve SimplifyLocals pass

Temporarily rebased on top of #66074. The top 2 commits are new.

r? @oli-obk
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 10, 2019
…ovements, r=oli-obk

[mir-opt] Handle return place in ConstProp and improve SimplifyLocals pass

Temporarily rebased on top of rust-lang#66074. The top 2 commits are new.

r? @oli-obk
perf.rlo shows that running the `ConstProp` pass results in
across-the-board wins regardless of debug or opt complilation mode. As a
result, we're turning it on to get the compile time benefits.

`ConstProp` doesn't currently intern the memory used by its `Machine` so
we can't yet propagate allocations which is why
`ConstProp::should_const_prop()` checks if the value being propagated is
a scalar or not.
@wesleywiser wesleywiser force-pushed the wesleywiser:test_run_const_prop branch from db23021 to db5fc10 Nov 12, 2019
@wesleywiser wesleywiser changed the title [experiment] Test ConstProp performance [mir-opt] Turn on the `ConstProp` pass by default Nov 12, 2019
@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Nov 12, 2019

This is ready for a final review.

r? @oli-obk

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Nov 12, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 12, 2019

📌 Commit db5fc10 has been approved by oli-obk

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 13, 2019
…oli-obk

[mir-opt] Turn on the `ConstProp` pass by default

perf.rlo shows that running the `ConstProp` pass results in
across-the-board wins regardless of debug or opt complilation mode. As a
result, we're turning it on to get the compile time benefits.
bors added a commit that referenced this pull request Nov 13, 2019
Rollup of 13 pull requests

Successful merges:

 - #65932 (download .tar.xz if python3 is used)
 - #66074 ([mir-opt] Turn on the `ConstProp` pass by default)
 - #66094 (Fix documentation for `Iterator::count()`.)
 - #66166 (rename cfg(rustdoc) into cfg(doc))
 - #66227 (docs: Fix link to BufWriter::flush)
 - #66292 (add Result::map_or)
 - #66297 (Add a callback that allows compiler consumers to override queries.)
 - #66317 (Use a relative bindir for rustdoc to find rustc)
 - #66330 (Improve non-exhaustiveness handling in usefulness checking)
 - #66331 (Add some tests for fixed ICEs)
 - #66334 (Move Session fields to CrateStore)
 - #66335 (Move self-profile infrastructure to data structures)
 - #66337 (Remove dead code for encoding/decoding lint IDs)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Nov 13, 2019
Rollup of 13 pull requests

Successful merges:

 - #65932 (download .tar.xz if python3 is used)
 - #66074 ([mir-opt] Turn on the `ConstProp` pass by default)
 - #66094 (Fix documentation for `Iterator::count()`.)
 - #66166 (rename cfg(rustdoc) into cfg(doc))
 - #66227 (docs: Fix link to BufWriter::flush)
 - #66292 (add Result::map_or)
 - #66297 (Add a callback that allows compiler consumers to override queries.)
 - #66317 (Use a relative bindir for rustdoc to find rustc)
 - #66330 (Improve non-exhaustiveness handling in usefulness checking)
 - #66331 (Add some tests for fixed ICEs)
 - #66334 (Move Session fields to CrateStore)
 - #66335 (Move self-profile infrastructure to data structures)
 - #66337 (Remove dead code for encoding/decoding lint IDs)

Failed merges:

r? @ghost
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 13, 2019
…oli-obk

[mir-opt] Turn on the `ConstProp` pass by default

perf.rlo shows that running the `ConstProp` pass results in
across-the-board wins regardless of debug or opt complilation mode. As a
result, we're turning it on to get the compile time benefits.
bors added a commit that referenced this pull request Nov 13, 2019
Rollup of 16 pull requests

Successful merges:

 - #65821 (Use `drop_in_place` in `array::IntoIter::drop`)
 - #65932 (download .tar.xz if python3 is used)
 - #66074 ([mir-opt] Turn on the `ConstProp` pass by default)
 - #66094 (Fix documentation for `Iterator::count()`.)
 - #66166 (rename cfg(rustdoc) into cfg(doc))
 - #66186 (Add long error explanation for E0623)
 - #66227 (docs: Fix link to BufWriter::flush)
 - #66248 (add raw ptr variant of UnsafeCell::get)
 - #66292 (add Result::map_or)
 - #66297 (Add a callback that allows compiler consumers to override queries.)
 - #66317 (Use a relative bindir for rustdoc to find rustc)
 - #66330 (Improve non-exhaustiveness handling in usefulness checking)
 - #66331 (Add some tests for fixed ICEs)
 - #66334 (Move Session fields to CrateStore)
 - #66335 (Move self-profile infrastructure to data structures)
 - #66337 (Remove dead code for encoding/decoding lint IDs)

Failed merges:

r? @ghost
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 13, 2019

@bors rollup=never

@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Nov 13, 2019

@bors r-

I'd like to resolve #66342 before this is merged.

@joelpalmer

This comment has been minimized.

Copy link

joelpalmer commented Nov 18, 2019

Ping from Triage: Hi @wesleywiser I see #66342 was closed yesterday. Please update this PR when you have a chance. Thanks!

@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Nov 19, 2019

@bors r=oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

📌 Commit db5fc10 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

⌛️ Testing commit db5fc10 with merge d1da802...

bors added a commit that referenced this pull request Nov 19, 2019
[mir-opt] Turn on the `ConstProp` pass by default

perf.rlo shows that running the `ConstProp` pass results in
across-the-board wins regardless of debug or opt complilation mode. As a
result, we're turning it on to get the compile time benefits.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 19, 2019

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

@bors bors added the merged-by-bors label Nov 19, 2019
@bors bors merged commit db5fc10 into rust-lang:master Nov 19, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191112.3 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a8 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a8 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
Rustup to rustc 1.41.0-nightly (35ef33a8 2019-11-21)

I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3

List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.