Optimize matches #60730
Optimize matches #60730
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
|
☀️ Try build successful - checks-travis |
|
@rust-lang/infra Can I have a perf run? |
|
@rust-timer build df9b1d3 |
|
Success: Queued df9b1d3 with parent b8e0d0a, comparison URL. |
|
Finished benchmarking try commit df9b1d3 |
src/librustc/mir/mod.rs
Outdated
| @@ -1180,9 +1180,9 @@ pub enum TerminatorKind<'tcx> { | |||
| FalseEdges { | |||
| /// The target normal control flow will take | |||
| real_target: BasicBlock, | |||
| /// The list of blocks control flow could conceptually take, but won't | |||
| /// A block control flow could conceptually take, but won't | |||
There was a problem hiding this comment.
| /// A block control flow could conceptually take, but won't | |
| /// A block control flow could conceptually jump to, but won't |
| adt_def.variants.len() + 1 | ||
| } | ||
| TestKind::SwitchInt { switch_ty, ref options, .. } => { | ||
| if switch_ty.is_bool() { |
There was a problem hiding this comment.
I'm trying to figure out the overall picture here.
My current best guess is that, for SwitchInt on a bool, the options ends up holding just some permutation of { 0, 1 } (and we know that to be exhaustive, since the type is bool), and thus options.len() + 1 would give the wrong number of target basic blocks.
Regardless of whether my hypothesis above is correct or incorrect, it might be nice to add some docs to SwitchInt explaining the role of the options field, and what invariants it upholds with respect to the other fields (switch_ty and indices).
| // StorageLive(_4); | ||
| // _4 = _2; | ||
| // ((_0 as A).0: i32) = move _4; | ||
| // discriminant(_0) = 0; |
There was a problem hiding this comment.
(I find this effect on the generated MIR interesting. I don't know whether its bad or good, in terms of code quality i.e. the ability of LLVM to optimize the result)
There was a problem hiding this comment.
The diff is a little messed up, all that's happening here is the two blocks are being swapped
| @@ -28,16 +28,16 @@ LL | let x; | |||
| ... | |||
| LL | x = 1; | |||
| | ^^^^^ cannot assign twice to immutable variable | |||
| LL | } else { | |||
| LL | x = 2; | |||
| | ----- first assignment to `x` | |||
There was a problem hiding this comment.
(it always makes me a little sad to see leaking of the reality that our static analysis is happening on a control-flow graph that does not always bias itself to act like syntactic order should be favored when the control-flow is otherwise semi-arbitrary. But there are other instances where things like this leak through and its certainly not your job, or maybe anyone's job, to address it.)
|
The perf results look entirely acceptable to me. |
|
@matthewjasper maybe I missed it, but did you add a test case for the actual problem described in #60571 somewhere in this PR? |
|
This all seems fine to me. I left some general feedback, but I don't see a problem with landing this PR as is. Having said that, we can't merge draft pull requests. So I'll just say: r=me, and switch this to S-waiting-on-author so that @matthewjasper can decide if its fully-baked, or if it needs more changes and then another round of review. |
pnkfelix
added
S-waiting-on-author
I am surprised that things don't look better tho... I would have expected to regain more of the loss. The mir-opt test for
I don't see a mir-opt test for |
|
Just to give an update here: it's taken me awhile to get back to this. I've had a look at the |
matthewjasper
force-pushed
the
optimize-false-edges
branch
from
d7f31a6 to
cafb9d7
Compare
|
☔ The latest upstream changes (presumably #59276) made this pull request unmergeable. Please resolve the merge conflicts. |
matthewjasper
force-pushed
the
optimize-false-edges
branch
2 times, most recently
from
9425a84 to
967d806
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
matthewjasper
force-pushed
the
optimize-false-edges
branch
from
967d806 to
2f57a82
Compare
|
@bors try |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 89ea69a with merge 8e774d33f9956f3e6a69fb85878dc754cb879f63... |
|
💔 Test failed - checks-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors retry spurious 30 min |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-travis, status-appveyor |
Tested on commit rust-lang/rust@6865502. Direct link to PR: <rust-lang/rust#60730> 💔 rls on windows: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).
|
@matthewjasper what benchmark did you use to measure the impact of |
|
I can't really remember using anything other than the perf.rlo benchmarks. Maybe I had a test that consisted only of a very large match against a lot of string literals. |
|
Oh wait, your commit says "spend less time optimizing", so it was a compile time problem! The |
… r=<try> match lowering: Remove the `make_target_blocks` hack This hack was introduced 4 years ago in [`a1d0266` (rust-lang#60730)](rust-lang@a1d0266) to improve LLVM optimization time, specifically noticed in the `encoding` benchmark. Measurements today indicate it is no longer needed. r? `@matthewjasper`
… r=matthewjasper match lowering: Remove the `make_target_blocks` hack This hack was introduced 4 years ago in [`a1d0266` (rust-lang#60730)](rust-lang@a1d0266) to improve LLVM optimization time, specifically noticed in the `encoding` benchmark. Measurements today indicate it is no longer needed. r? `@matthewjasper`
Attempt to fix or improve #60571
This is breaking some diagnostics because the MIR for match arms isn't in source order any more.
cc @Centril