MatchBranchSimplification: fix equal const bool assignments #75537
Conversation
It might be necessary to use its value more than once.
rust-highfive
added
the
S-waiting-on-review
tmiasko
changed the title
tmiasko
force-pushed
the
match-branch-simplify
branch
2 times, most recently
from
621c973 to
c96c5a0
Compare
| && f_c.literal.ty.is_bool() | ||
| && s_c.literal.ty.is_bool() | ||
| && f_c.literal.try_eval_bool(tcx, param_env).is_some() | ||
| && s_c.literal.try_eval_bool(tcx, param_env).is_some() => {} |
There was a problem hiding this comment.
I feel like these two stmts will always evaluate to true if the previous two did, but better safe than sorry
| @@ -78,7 +78,7 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification { | |||
| ); | |||
| if let Some(c) = c.literal.try_eval_bool(tcx, param_env) { | |||
| let op = if c { BinOp::Eq } else { BinOp::Ne }; | |||
| *rhs = Rvalue::BinaryOp(op, Operand::Move(discr), const_cmp); | |||
| *rhs = Rvalue::BinaryOp(op, Operand::Copy(discr), const_cmp); | |||
There was a problem hiding this comment.
Should this mirror the type that is matched in discr, rather than always be Copy?
There was a problem hiding this comment.
AFAIU move leaves undefined data behind and moving from it again would be undefined behaviour. At least in theory, if not currently in practice. Since we might need to use it more than once, using copy should be appropriate.
There was a problem hiding this comment.
Ah makes sense, I wonder if this should always be done if the enum is relatively large
tmiasko
force-pushed
the
match-branch-simplify
branch
from
c96c5a0 to
1fe2e29
Compare
| @@ -48,7 +48,7 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification { | |||
| ref targets, | |||
| ref values, | |||
| .. | |||
| } if targets.len() == 2 && values.len() == 1 => { | |||
| } if targets.len() == 2 && values.len() == 1 && targets[0] != targets[1] => { | |||
There was a problem hiding this comment.
Just curious, is targets[0] == targets[1] ever hit? I imagine that would be a whole different opt pass
There was a problem hiding this comment.
I don't know.
The CfgSimplifier optimizes switch with the same targets to a goto, and then collapses goto chains and merges blocks.
There was a problem hiding this comment.
we can make this condition an assert after the beta branch, then we have 12 weeks to find out if it's a problem :D
The match branch simplification is applied when target blocks contain statements that are either equal or perform a const bool assignment with different values to the same place. Previously, when constructing new statements, only statements from a single block had been examined. This lead to a misoptimization when statements are equal because the assign the *same* const bool value to the same place. Fix the issue by examining statements from both blocks when deciding on replacement.
|
r=me with the unsafe code replaced with a safe solution |
|
@bors r+ |
|
📌 Commit af9b9e4 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
The match branch simplification is applied when target blocks contain
statements that are either equal or perform a const bool assignment with
different values to the same place.
Previously, when constructing new statements, only statements from a
single block had been examined. This lead to a misoptimization when
statements are equal because the assign the same const bool value to
the same place.
Fix the issue by examining statements from both blocks when deciding on
replacement.
Additionally:
value more than once.
Based on #75508.
r? @oli-obk / @JulianKnodt