-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Merge these copy statements that simplified the canonical enum clone method by GVN #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. |
This reverts commit 25d434b.
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?
if simplify_to_copy(tcx, body, bb_idx, param_env).is_some() { | ||
should_cleanup = true; | ||
continue; | ||
} |
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 be made a standalone MirPass? This will be easier to have some analyses computed only once.
if stmts.is_empty() { | ||
return None; | ||
} | ||
if let [Statement { kind: StatementKind::Assign(box (place, rvalue)), .. }] = |
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.
Could you add a comment for the high-level pattern you are trying to match?
} | ||
} | ||
} | ||
let expected_copy_stmt = expected_copy_stmt?; |
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 perform the same analysis on this statement that we do in the single statement branch above?
It looks like makes sense. @rustbot author |
This is blocked by #128299.