-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Simplify the canonical enum clone branches to a copy statement #148034
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
base: main
Are you sure you want to change the base?
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
[WIP] Simplify the canonical enum clone branches to a copy statement
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
💔 Test for b4269e3 failed: CI. Failed jobs:
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
[WIP] Simplify the canonical enum clone branches to a copy statement
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (673e3f6): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.4%, secondary -2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 476.496s -> 474.095s (-0.50%) |
d2deaa3 to
8a2e9e4
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Simplify the canonical enum clone branches to a copy statement
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (da8af3c): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.08s -> 474.068s (-0.21%) |
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
r? cjgillot |
Yeah, that's probably a better choice, I'm not that familiar with MIR. |
| let typing_env = body.typing_env(tcx); | ||
| let mut apply_patch = false; | ||
| let mut patch = MirPatch::new(body); | ||
| for (bb, bb_data) in body.basic_blocks.iter_enumerated() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing: we examine terminators. Should we traverse the body in post-order? And have a patch for each iteration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| let new_stmts = if body.basic_blocks[first_case].statements.is_empty() { | ||
| Vec::new() | ||
| } else if let Some(else_target) = as_else_target | ||
| && let Some(new_stmts) = simplify_if( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be made a method on SimplifyMatch? Or keep simplify_match a simple function that constructs a SimplifyMatch helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adopt a more general approach.
| (*first, others) | ||
| } | ||
| _ => return None, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return None if other_cases is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new changes should be able to treat this as a general case.
| let bbs = &self.body.basic_blocks; | ||
| // Check if the copy source matches the following pattern. | ||
| // _2 = discriminant(*_1); // "*_1" is the expected the copy source. | ||
| // switchInt(move _2) -> [0: bb3, 1: bb2, otherwise: bb1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we check aliasing? To ensure that the enum *_1 is not changed between the switch and the current statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I missed. I checked whether the statement is the first statement.
I have overhauled MatchBranchSimplification in this PR. This pass is tested merge cases one by one, which is more readable and extensible.
This PR also merges the following pattern that is mostly generated by GVN into one basic block that contains the copy statement:
Fixes #128081.