Skip to content

Commit

Permalink
Rollup merge of #89532 - ecstatic-morse:maybe-live-locals-enum, r=oli…
Browse files Browse the repository at this point in the history
…-obk,tmiasko

Document behavior of  `MaybeLiveLocals` regarding enums and field-senstivity

This arose from a [discussion on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/MaybeLiveLocals.20and.20Discriminants) where a new contributor attempted to implement a dead-store elimination pass using this analysis. They ran into a nasty hack around `SetDiscriminant` the effect of which is to lets handle assignments of literals to enum-typed locals (e.g. `x = Some(4)`) correctly. This took me a while to figure out.

Document this oddity, so the next person will have an easier time, and add a test to enshrine the current behavior.

r? `@tmiasko`
  • Loading branch information
Manishearth authored Oct 5, 2021
2 parents d567460 + 9f9f7f6 commit e8f2cdd
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 0 deletions.
31 changes: 31 additions & 0 deletions compiler/rustc_mir_dataflow/src/impls/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,37 @@ use crate::{AnalysisDomain, Backward, GenKill, GenKillAnalysis};
/// exist. See [this `mir-dataflow` test][flow-test] for an example. You almost never want to use
/// this analysis without also looking at the results of [`MaybeBorrowedLocals`].
///
/// ## Field-(in)sensitivity
///
/// As the name suggests, this analysis is field insensitive. If a projection of a variable `x` is
/// assigned to (e.g. `x.0 = 42`), it does not "define" `x` as far as liveness is concerned. In fact,
/// such an assignment is currently marked as a "use" of `x` in an attempt to be maximally
/// conservative.
///
/// ## Enums and `SetDiscriminant`
///
/// Assigning a literal value to an `enum` (e.g. `Option<i32>`), does not result in a simple
/// assignment of the form `_1 = /*...*/` in the MIR. For example, the following assignment to `x`:
///
/// ```
/// x = Some(4);
/// ```
///
/// compiles to this MIR
///
/// ```
/// ((_1 as Some).0: i32) = const 4_i32;
/// discriminant(_1) = 1;
/// ```
///
/// However, `MaybeLiveLocals` **does** mark `x` (`_1`) as "killed" after a statement like this.
/// That's because it treats the `SetDiscriminant` operation as a definition of `x`, even though
/// the writes that actually initialized the locals happened earlier.
///
/// This makes `MaybeLiveLocals` unsuitable for certain classes of optimization normally associated
/// with a live variables analysis, notably dead-store elimination. It's a dirty hack, but it works
/// okay for the generator state transform (currently the main consumuer of this analysis).
///
/// [`MaybeBorrowedLocals`]: super::MaybeBorrowedLocals
/// [flow-test]: https://github.com/rust-lang/rust/blob/a08c47310c7d49cbdc5d7afb38408ba519967ecd/src/test/ui/mir-dataflow/liveness-ptr.rs
/// [liveness]: https://en.wikipedia.org/wiki/Live_variable_analysis
Expand Down
22 changes: 22 additions & 0 deletions src/test/ui/mir-dataflow/liveness-enum.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#![feature(core_intrinsics, rustc_attrs)]

use std::intrinsics::rustc_peek;

#[rustc_mir(rustc_peek_liveness, stop_after_dataflow)]
fn foo() -> Option<i32> {
let mut x = None;

// `x` is live here since it is used in the next statement...
rustc_peek(x);

dbg!(x);

// But not here, since it is overwritten below
rustc_peek(x); //~ ERROR rustc_peek: bit not set

x = Some(4);

x
}

fn main() {}
10 changes: 10 additions & 0 deletions src/test/ui/mir-dataflow/liveness-enum.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: rustc_peek: bit not set
--> $DIR/liveness-enum.rs:15:5
|
LL | rustc_peek(x);
| ^^^^^^^^^^^^^

error: stop_after_dataflow ended compilation

error: aborting due to 2 previous errors

0 comments on commit e8f2cdd

Please sign in to comment.