-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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: Ignore the dead store statements in MatchBranchSimplification #129931
base: master
Are you sure you want to change the base?
Conversation
r? @nnethercote rustbot has assigned @nnethercote. Use |
r? ghost |
Failed to set assignee to
|
r? cjgillot |
This comment has been minimized.
This comment has been minimized.
efce2e8
to
adb6166
Compare
☔ The latest upstream changes (presumably #128299) made this pull request unmergeable. Please resolve the merge conflicts. |
adb6166
to
622247a
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Merge these copy statements that simplified the canonical enum clone method by GVN This is blocked by rust-lang#128299.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c324112): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 0.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 0.0%, secondary -0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 760.71s -> 756.595s (-0.54%) |
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.
My high-level reaction to this PR is that it's both too specific in what it's trying to match (a clone impl) and too general in its implementation (handles storage statements...).
I suggest having two heuristics:
- for clone impls, use some kind of
StructuralClone
trait so that we fully replace the derived implsClone
with simpler MIR; - for general MIR, lift the restrictions on this pass (bb0 and assignment to _0 namely).
What do you think?
@@ -604,6 +602,7 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { | |||
&dead_store_elimination::DeadStoreElimination::Initial, | |||
&gvn::GVN, | |||
&simplify::SimplifyLocals::AfterGVN, | |||
&match_branches::MatchBranchSimplification, |
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 this run on clone shims too? Or do we want a trait-based solution to detect trivial clone impls?
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 this run on clone shims too?
Could you explain more?
Or do we want a trait-based solution to detect trivial clone impls?
It makes sense to me for clone impls.
It looks like makes sense. @rustbot author |
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 didn't submit all the comments through "Submit review"; there are still some pending comments. o
I am currently modifying the code to move it into a separate pass.
@@ -604,6 +602,7 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { | |||
&dead_store_elimination::DeadStoreElimination::Initial, | |||
&gvn::GVN, | |||
&simplify::SimplifyLocals::AfterGVN, | |||
&match_branches::MatchBranchSimplification, |
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 this run on clone shims too?
Could you explain more?
Or do we want a trait-based solution to detect trivial clone impls?
It makes sense to me for clone impls.
Just rebased. Perhaps I should move the code in @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
mir-opt: Merge all branch BBs into a single copy statement rust-lang#128299 simplified ```rust match a { Foo::A(x) => Foo::A(*x), Foo::B => Foo::B } ``` to ```rust match a { Foo::A(x) => a, // copy a Foo::B => Foo::B } ``` The switch branch can be simplified into a single copy statement. This PR implements a relatively general simplification.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (27bc686): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -3.8%, secondary -2.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 1.8%, secondary 2.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.1%, secondary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 778.988s -> 776.478s (-0.32%) |
let mut patch = MirPatch::new(body); | ||
patch.add_assign(parent_end, dest_place, Rvalue::Use(Operand::Copy(src_place))); | ||
patch.patch_terminator(switch_bb_idx, first_terminator_kind.clone()); | ||
patch.apply(body); | ||
super::simplify::remove_dead_blocks(body); | ||
// After modifying the MIR, the result of `MaybeTransitiveLiveLocals` may become invalid, | ||
// keeping it simple to process only once. | ||
break; |
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'd recommend having a single patch and modify MIR at the end. This allows to catch several instances of the opt in the same body.
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 fixed this in 8105aefe072e1dffbcc2f0d9114e48c9add6d03b.
@@ -609,6 +610,7 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { | |||
&dead_store_elimination::DeadStoreElimination::Initial, | |||
&gvn::GVN, | |||
&simplify::SimplifyLocals::AfterGVN, | |||
&merge_branches::MergeBranchSimplification, |
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 add a dead_store_elimination::DeadStoreElimination::AfterGVN
just before it and simplify your pass?
return None; | ||
} | ||
} | ||
Rvalue::Use(Operand::Copy(place)) if *place == src_place => {} |
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.
What about Operand::Move
? Downgrading a move to a copy is fine.
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'll address it in the next PR or a subsequent update.
To better handle dead store statements, I'm cloning basic blocks in I haven't transformed @rustbot review |
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
Reminder, once the PR becomes ready for a review, use |
I should drop other dead store statements or not merging this match: pub fn foo(b: bool) {
let _b = (if b { " " } else { "" },);
}
fn foo(_1: bool) -> () {
debug b => _1;
let mut _0: ();
let _2: (&str,);
let mut _3: &str;
scope 1 {
debug _b => _2;
}
bb0: {
switchInt(copy _1) -> [0: bb2, otherwise: bb1];
}
bb1: {
_3 = const " ";
goto -> bb3;
}
bb2: {
_3 = const "";
goto -> bb3;
}
bb3: {
_2 = (move _3,);
return;
}
} I'm considering not merging this match. |
#128299 simplified
to
Since all of these are dead store statements, we can simplify and merge them into a single copy statement.