Less cloning in early otherwise branch #77472
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
jyn514
added
the
T-compiler
This comment has been minimized.
This comment has been minimized.
The same condition is checked just above
simonvandel
force-pushed
the
refactor-early-otherwise-branch
branch
from
1106178 to
1a8d28a
Compare
jyn514
added
C-cleanup
| @@ -300,7 +299,6 @@ impl<'a, 'tcx> Helper<'a, 'tcx> { | |||
| // the declaration of the discriminant read. Place of this read is being used in the switch | |||
| let discr_decl = &self.body.local_decls()[discr_local]; | |||
| let discr_ty = discr_decl.ty; | |||
| // the otherwise target lies as the last element | |||
There was a problem hiding this comment.
I don't think you meant to remove this comment.
| // check that the value being matched on is the same. The | ||
| if this_bb_discr_info.targets_with_values.iter().find(|x| x.0 == value).is_none() { |
There was a problem hiding this comment.
Where is this check duplicated? The check above does not look at targets_with_values at all.
| // The types of the two adts matched on have to be equal for this optimization to apply | ||
| if discr_info.type_adt_matched_on != this_bb_discr_info.type_adt_matched_on { | ||
| trace!( | ||
| "NO: types do not match. LHS: {:?}, RHS: {:?}", | ||
| discr_info.type_adt_matched_on, | ||
| this_bb_discr_info.type_adt_matched_on | ||
| ); | ||
| return None; | ||
| } |
There was a problem hiding this comment.
There doesn't seem to be much point to this change without benchmarks. Personally I would expect a simple != to be faster than two conditionals.
| @@ -198,36 +196,36 @@ struct OptimizationToApply<'tcx> { | |||
| struct OptimizationInfo<'tcx> { | |||
| /// Info about the first switch and discriminant | |||
| first_switch_info: SwitchDiscriminantInfo<'tcx>, | |||
| /// Info about the second switch and discriminant | |||
| second_switch_info: SwitchDiscriminantInfo<'tcx>, | |||
| /// Info about all swtiches that are successors of the first switch | |||
There was a problem hiding this comment.
| /// Info about all swtiches that are successors of the first switch | |
| /// Info about all switches that are successors of the first switch |
camelid
added
S-waiting-on-review
|
r? @oli-obk |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 1a8d28a with merge 556d995f6cd76d0ded6c51fa190e2ebf48671659... |
|
☀️ Try build successful - checks-actions |
|
Queued 556d995f6cd76d0ded6c51fa190e2ebf48671659 with parent 0d96516, future comparison URL. |
|
Finished benchmarking try commit (556d995f6cd76d0ded6c51fa190e2ebf48671659): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
No measurable impact. I'll go over the code later, but I think we should thus only take the commits that improve readability |
|
@simonvandel Could you address Oli's comments? By the way, you have some merge conflicts. |
camelid
added
S-waiting-on-author
|
I currently don't have the time to work on this, unfortunately. If you prefer to close it for now, then feel tree to do so. |
Dylan-DPC-zz
added
the
S-inactive
This is probably easiest read commit by commit.