Skip to content

Commit

Permalink
Rollup merge of rust-lang#79882 - wecing:master, r=oli-obk
Browse files Browse the repository at this point in the history
Fix issue rust-lang#78496

EarlyOtherwiseBranch finds MIR structures like:

```
bb0: {
  ...
  _2 = discriminant(X)
  ...
  switchInt(_2) -> [1_isize: bb1, otherwise: bb3]
}
bb1: {
  ...
  _3 = discriminant(Y)
  ...
  switchInt(_3) -> [1_isize: bb2, otherwise: bb3]
}
bb2: {...}
bb3: {...}
```

And transforms them into something like:

```
bb0: {
  ...
  _2 = discriminant(X)
  _3 = discriminant(Y)
  _4 = Eq(_2, _3)
  switchInt(_4) -> [true: bb4, otherwise: bb3]
}
bb2: {...} // unchanged
bb3: {...} // unchanged
bb4: {
  switchInt(_2) -> [1_isize: bb2, otherwise: bb3]
}
```

But that is not always a safe thing to do -- sometimes the early `otherwise` branch is necessary so the later block could assume the value of `discriminant(X)`.

I am not totally sure what's the best way to detect that, but fixing rust-lang#78496 should be easy -- we just check if `X` is a sub-expression of `Y`. A more precise test might be to check if `Y` contains a `Downcast(1)` of `X`, but I think this might be good enough.

Fix rust-lang#78496
  • Loading branch information
Dylan-DPC authored Dec 16, 2020
2 parents ed1f0b5 + 3812f70 commit 864d804
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 0 deletions.
27 changes: 27 additions & 0 deletions compiler/rustc_mir/src/transform/early_otherwise_branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,33 @@ impl<'a, 'tcx> Helper<'a, 'tcx> {
return None;
}

// when the second place is a projection of the first one, it's not safe to calculate their discriminant values sequentially.
// for example, this should not be optimized:
//
// ```rust
// enum E<'a> { Empty, Some(&'a E<'a>), }
// let Some(Some(_)) = e;
// ```
//
// ```mir
// bb0: {
// _2 = discriminant(*_1)
// switchInt(_2) -> [...]
// }
// bb1: {
// _3 = discriminant(*(((*_1) as Some).0: &E))
// switchInt(_3) -> [...]
// }
// ```
let discr_place = discr_info.place_of_adt_discr_read;
let this_discr_place = this_bb_discr_info.place_of_adt_discr_read;
if discr_place.local == this_discr_place.local
&& this_discr_place.projection.starts_with(discr_place.projection)
{
trace!("NO: one target is the projection of another");
return None;
}

// if we reach this point, the optimization applies, and we should be able to optimize this case
// store the info that is needed to apply the optimization

Expand Down
16 changes: 16 additions & 0 deletions src/test/ui/mir/issue-78496.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// run-pass
// compile-flags: -Z mir-opt-level=2 -C opt-level=0

// example from #78496
pub enum E<'a> {
Empty,
Some(&'a E<'a>),
}

fn f(e: &E) -> u32 {
if let E::Some(E::Some(_)) = e { 1 } else { 2 }
}

fn main() {
assert_eq!(f(&E::Empty), 2);
}

0 comments on commit 864d804

Please sign in to comment.