Skip to content

Commit

Permalink
Auto merge of rust-lang#71956 - ecstatic-morse:remove-requires-storag…
Browse files Browse the repository at this point in the history
…e-analysis, r=tmandry

Clean up logic around live locals in generator analysis

Resolves rust-lang#69902. Requires rust-lang#71893.

I've found it difficult to make changes in the logic around live locals in `generator/transform.rs`. It uses a custom dataflow analysis, `MaybeRequiresStorage`, that AFAICT computes whether a local is either initialized or borrowed. That analysis is using `before` effects, which we should try to avoid if possible because they are harder to reason about than ones only using the unprefixed effects. @pnkfelix has suggested removing "before" effects entirely to simplify the dataflow framework, which I might pursue someday.

This PR replaces `MaybeRequiresStorage` with a combination of the existing `MaybeBorrowedLocals` and a new `MaybeInitializedLocals`. `MaybeInitializedLocals` is just `MaybeInitializedPlaces` with a coarser resolution: it works on whole locals instead of move paths. As a result, I was able to simplify the logic in `compute_storage_conflicts` and `locals_live_across_suspend_points`.

This is not exactly equivalent to the old logic; some generators are now smaller than before. I believe this was because the old logic was too conservative, but I'm not as familiar with the constraints as the original implementers were, so I could be wrong. For example, I don't see a reason the size of the `mixed_sizes` future couldn't be 5K. It went from 7K to 6K in this PR.

r? @jonas-schievink @tmandry
  • Loading branch information
bors committed May 22, 2020
2 parents d9417b3 + 3ff9317 commit 458a3e7
Show file tree
Hide file tree
Showing 7 changed files with 254 additions and 372 deletions.
3 changes: 3 additions & 0 deletions src/librustc_mir/dataflow/impls/borrowed_locals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ impl<K> GenKillAnalysis<'tcx> for MaybeBorrowedLocals<K>
where
K: BorrowAnalysisKind<'tcx>,
{
// The generator transform relies on the fact that this analysis does **not** use "before"
// effects.

fn statement_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
Expand Down
118 changes: 118 additions & 0 deletions src/librustc_mir/dataflow/impls/init_locals.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
//! A less precise version of `MaybeInitializedPlaces` whose domain is entire locals.
//!
//! A local will be maybe initialized if *any* projections of that local might be initialized.

use crate::dataflow::{self, BottomValue, GenKill};

use rustc_index::bit_set::BitSet;
use rustc_middle::mir::visit::{PlaceContext, Visitor};
use rustc_middle::mir::{self, BasicBlock, Local, Location};

pub struct MaybeInitializedLocals;

impl BottomValue for MaybeInitializedLocals {
/// bottom = uninit
const BOTTOM_VALUE: bool = false;
}

impl dataflow::AnalysisDomain<'tcx> for MaybeInitializedLocals {
type Idx = Local;

const NAME: &'static str = "maybe_init_locals";

fn bits_per_block(&self, body: &mir::Body<'tcx>) -> usize {
body.local_decls.len()
}

fn initialize_start_block(&self, body: &mir::Body<'tcx>, entry_set: &mut BitSet<Self::Idx>) {
// Function arguments are initialized to begin with.
for arg in body.args_iter() {
entry_set.insert(arg);
}
}
}

impl dataflow::GenKillAnalysis<'tcx> for MaybeInitializedLocals {
// The generator transform relies on the fact that this analysis does **not** use "before"
// effects.

fn statement_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
statement: &mir::Statement<'tcx>,
loc: Location,
) {
TransferFunction { trans }.visit_statement(statement, loc)
}

fn terminator_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
terminator: &mir::Terminator<'tcx>,
loc: Location,
) {
TransferFunction { trans }.visit_terminator(terminator, loc)
}

fn call_return_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
_block: BasicBlock,
_func: &mir::Operand<'tcx>,
_args: &[mir::Operand<'tcx>],
return_place: mir::Place<'tcx>,
) {
trans.gen(return_place.local)
}

/// See `Analysis::apply_yield_resume_effect`.
fn yield_resume_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
_resume_block: BasicBlock,
resume_place: mir::Place<'tcx>,
) {
trans.gen(resume_place.local)
}
}

struct TransferFunction<'a, T> {
trans: &'a mut T,
}

impl<T> Visitor<'tcx> for TransferFunction<'a, T>
where
T: GenKill<Local>,
{
fn visit_local(&mut self, &local: &Local, context: PlaceContext, _: Location) {
use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, NonUseContext};
match context {
// These are handled specially in `call_return_effect` and `yield_resume_effect`.
PlaceContext::MutatingUse(MutatingUseContext::Call | MutatingUseContext::Yield) => {}

// Otherwise, when a place is mutated, we must consider it possibly initialized.
PlaceContext::MutatingUse(_) => self.trans.gen(local),

// If the local is moved out of, or if it gets marked `StorageDead`, consider it no
// longer initialized.
PlaceContext::NonUse(NonUseContext::StorageDead)
| PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) => self.trans.kill(local),

// All other uses do not affect this analysis.
PlaceContext::NonUse(
NonUseContext::StorageLive
| NonUseContext::AscribeUserTy
| NonUseContext::VarDebugInfo,
)
| PlaceContext::NonMutatingUse(
NonMutatingUseContext::Inspect
| NonMutatingUseContext::Copy
| NonMutatingUseContext::SharedBorrow
| NonMutatingUseContext::ShallowBorrow
| NonMutatingUseContext::UniqueBorrow
| NonMutatingUseContext::AddressOf
| NonMutatingUseContext::Projection,
) => {}
}
}
}
4 changes: 3 additions & 1 deletion src/librustc_mir/dataflow/impls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,15 @@ use crate::dataflow::drop_flag_effects;

mod borrowed_locals;
pub(super) mod borrows;
mod init_locals;
mod liveness;
mod storage_liveness;

pub use self::borrowed_locals::{MaybeBorrowedLocals, MaybeMutBorrowedLocals};
pub use self::borrows::Borrows;
pub use self::init_locals::MaybeInitializedLocals;
pub use self::liveness::MaybeLiveLocals;
pub use self::storage_liveness::{MaybeRequiresStorage, MaybeStorageLive};
pub use self::storage_liveness::MaybeStorageLive;

/// `MaybeInitializedPlaces` tracks all places that might be
/// initialized upon reaching a particular point in the control flow
Expand Down
234 changes: 1 addition & 233 deletions src/librustc_mir/dataflow/impls/storage_liveness.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
pub use super::*;

use crate::dataflow::BottomValue;
use crate::dataflow::{self, GenKill, Results, ResultsRefCursor};
use crate::dataflow::{self, GenKill};
use crate::util::storage::AlwaysLiveLocals;
use rustc_middle::mir::visit::{NonMutatingUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
use std::cell::RefCell;

#[derive(Clone)]
pub struct MaybeStorageLive {
Expand Down Expand Up @@ -78,233 +76,3 @@ impl BottomValue for MaybeStorageLive {
/// bottom = dead
const BOTTOM_VALUE: bool = false;
}

type BorrowedLocalsResults<'a, 'tcx> = ResultsRefCursor<'a, 'a, 'tcx, MaybeBorrowedLocals>;

/// Dataflow analysis that determines whether each local requires storage at a
/// given location; i.e. whether its storage can go away without being observed.
pub struct MaybeRequiresStorage<'mir, 'tcx> {
body: &'mir Body<'tcx>,
borrowed_locals: RefCell<BorrowedLocalsResults<'mir, 'tcx>>,
}

impl<'mir, 'tcx> MaybeRequiresStorage<'mir, 'tcx> {
pub fn new(
body: &'mir Body<'tcx>,
borrowed_locals: &'mir Results<'tcx, MaybeBorrowedLocals>,
) -> Self {
MaybeRequiresStorage {
body,
borrowed_locals: RefCell::new(ResultsRefCursor::new(&body, borrowed_locals)),
}
}
}

impl<'mir, 'tcx> dataflow::AnalysisDomain<'tcx> for MaybeRequiresStorage<'mir, 'tcx> {
type Idx = Local;

const NAME: &'static str = "requires_storage";

fn bits_per_block(&self, body: &mir::Body<'tcx>) -> usize {
body.local_decls.len()
}

fn initialize_start_block(&self, body: &mir::Body<'tcx>, on_entry: &mut BitSet<Self::Idx>) {
// The resume argument is live on function entry (we don't care about
// the `self` argument)
for arg in body.args_iter().skip(1) {
on_entry.insert(arg);
}
}
}

impl<'mir, 'tcx> dataflow::GenKillAnalysis<'tcx> for MaybeRequiresStorage<'mir, 'tcx> {
fn before_statement_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
stmt: &mir::Statement<'tcx>,
loc: Location,
) {
// If a place is borrowed in a statement, it needs storage for that statement.
self.borrowed_locals.borrow().analysis().statement_effect(trans, stmt, loc);

match &stmt.kind {
StatementKind::StorageDead(l) => trans.kill(*l),

// If a place is assigned to in a statement, it needs storage for that statement.
StatementKind::Assign(box (place, _))
| StatementKind::SetDiscriminant { box place, .. } => {
trans.gen(place.local);
}
StatementKind::LlvmInlineAsm(asm) => {
for place in &*asm.outputs {
trans.gen(place.local);
}
}

// Nothing to do for these. Match exhaustively so this fails to compile when new
// variants are added.
StatementKind::AscribeUserType(..)
| StatementKind::FakeRead(..)
| StatementKind::Nop
| StatementKind::Retag(..)
| StatementKind::StorageLive(..) => {}
}
}

fn statement_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
_: &mir::Statement<'tcx>,
loc: Location,
) {
// If we move from a place then only stops needing storage *after*
// that statement.
self.check_for_move(trans, loc);
}

fn before_terminator_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
terminator: &mir::Terminator<'tcx>,
loc: Location,
) {
// If a place is borrowed in a terminator, it needs storage for that terminator.
self.borrowed_locals.borrow().analysis().terminator_effect(trans, terminator, loc);

match &terminator.kind {
TerminatorKind::Call { destination: Some((place, _)), .. } => {
trans.gen(place.local);
}

// Note that we do *not* gen the `resume_arg` of `Yield` terminators. The reason for
// that is that a `yield` will return from the function, and `resume_arg` is written
// only when the generator is later resumed. Unlike `Call`, this doesn't require the
// place to have storage *before* the yield, only after.
TerminatorKind::Yield { .. } => {}

TerminatorKind::InlineAsm { operands, .. } => {
for op in operands {
match op {
InlineAsmOperand::Out { place, .. }
| InlineAsmOperand::InOut { out_place: place, .. } => {
if let Some(place) = place {
trans.gen(place.local);
}
}
InlineAsmOperand::In { .. }
| InlineAsmOperand::Const { .. }
| InlineAsmOperand::SymFn { .. }
| InlineAsmOperand::SymStatic { .. } => {}
}
}
}

// Nothing to do for these. Match exhaustively so this fails to compile when new
// variants are added.
TerminatorKind::Call { destination: None, .. }
| TerminatorKind::Abort
| TerminatorKind::Assert { .. }
| TerminatorKind::Drop { .. }
| TerminatorKind::DropAndReplace { .. }
| TerminatorKind::FalseEdges { .. }
| TerminatorKind::FalseUnwind { .. }
| TerminatorKind::GeneratorDrop
| TerminatorKind::Goto { .. }
| TerminatorKind::Resume
| TerminatorKind::Return
| TerminatorKind::SwitchInt { .. }
| TerminatorKind::Unreachable => {}
}
}

fn terminator_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
terminator: &mir::Terminator<'tcx>,
loc: Location,
) {
match &terminator.kind {
// For call terminators the destination requires storage for the call
// and after the call returns successfully, but not after a panic.
// Since `propagate_call_unwind` doesn't exist, we have to kill the
// destination here, and then gen it again in `call_return_effect`.
TerminatorKind::Call { destination: Some((place, _)), .. } => {
trans.kill(place.local);
}

// Nothing to do for these. Match exhaustively so this fails to compile when new
// variants are added.
TerminatorKind::Call { destination: None, .. }
| TerminatorKind::Yield { .. }
| TerminatorKind::Abort
| TerminatorKind::Assert { .. }
| TerminatorKind::Drop { .. }
| TerminatorKind::DropAndReplace { .. }
| TerminatorKind::FalseEdges { .. }
| TerminatorKind::FalseUnwind { .. }
| TerminatorKind::GeneratorDrop
| TerminatorKind::Goto { .. }
| TerminatorKind::InlineAsm { .. }
| TerminatorKind::Resume
| TerminatorKind::Return
| TerminatorKind::SwitchInt { .. }
| TerminatorKind::Unreachable => {}
}

self.check_for_move(trans, loc);
}

fn call_return_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
_block: BasicBlock,
_func: &mir::Operand<'tcx>,
_args: &[mir::Operand<'tcx>],
return_place: mir::Place<'tcx>,
) {
trans.gen(return_place.local);
}

fn yield_resume_effect(
&self,
trans: &mut impl GenKill<Self::Idx>,
_resume_block: BasicBlock,
resume_place: mir::Place<'tcx>,
) {
trans.gen(resume_place.local);
}
}

impl<'mir, 'tcx> MaybeRequiresStorage<'mir, 'tcx> {
/// Kill locals that are fully moved and have not been borrowed.
fn check_for_move(&self, trans: &mut impl GenKill<Local>, loc: Location) {
let mut visitor = MoveVisitor { trans, borrowed_locals: &self.borrowed_locals };
visitor.visit_location(&self.body, loc);
}
}

impl<'mir, 'tcx> BottomValue for MaybeRequiresStorage<'mir, 'tcx> {
/// bottom = dead
const BOTTOM_VALUE: bool = false;
}

struct MoveVisitor<'a, 'mir, 'tcx, T> {
borrowed_locals: &'a RefCell<BorrowedLocalsResults<'mir, 'tcx>>,
trans: &'a mut T,
}

impl<'a, 'mir, 'tcx, T> Visitor<'tcx> for MoveVisitor<'a, 'mir, 'tcx, T>
where
T: GenKill<Local>,
{
fn visit_local(&mut self, local: &Local, context: PlaceContext, loc: Location) {
if PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) == context {
let mut borrowed_locals = self.borrowed_locals.borrow_mut();
borrowed_locals.seek_before_primary_effect(loc);
if !borrowed_locals.contains(*local) {
self.trans.kill(*local);
}
}
}
}
Loading

0 comments on commit 458a3e7

Please sign in to comment.