Skip to content

Commit

Permalink
Auto merge of #96451 - JakobDegen:dest-prop, r=tmiasko
Browse files Browse the repository at this point in the history
Fix Dest Prop

Closes #82678, #79191 .

This was not originally a total re-write of the pass but is has gradually turned into one. Notable changes:

 1. Significant improvements to documentation all around. The top of the file has been extended with a more precise argument for soundness. The code should be fairly readable, and I've done my best to add useful comments wherever possible. I would very much like for the bus factor to not be one on this code.
 3. Improved handling of conflicts that are not visible in normal dataflow.  This was the cause of #79191. Handling this correctly requires us to make decision about the semantics and specifically evaluation order of basically all MIR constructs (see specifically #68364 #71117.  The way this is implemented is based on my preferred resolution to these questions around the semantics of assignment statements.
 4. Some re-architecting to improve performance. More details below.
 5. Possible future improvements to this optimization are documented, and the code is written with the needs of those improvements in mind. The hope is that adding support for more precise analyses will not require a full re-write of this opt, but just localized changes.

### Regarding Performance

The previous approach had some performance issues; letting `l` be the number of locals and `s` be the number of statements/terminators, the runtime of the pass was `O(l^2 * s)`, both in theory and in practice. This version is smarter about not calculating unnecessary things and doing more caching. Our runtime is now dominated by one invocation of `MaybeLiveLocals` for each "round," and the number of rounds is less than 5 in over 90% of cases. This means it's linear-ish in practice.

r? `@oli-obk` who reviewed the last version of this, but review from anyone else would be more than welcome
  • Loading branch information
bors committed Nov 27, 2022
2 parents faf1891 + 245c607 commit 0e9eee6
Show file tree
Hide file tree
Showing 40 changed files with 1,039 additions and 1,224 deletions.
122 changes: 0 additions & 122 deletions compiler/rustc_mir_dataflow/src/impls/init_locals.rs

This file was deleted.

2 changes: 0 additions & 2 deletions compiler/rustc_mir_dataflow/src/impls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,11 @@ use crate::{drop_flag_effects, on_all_children_bits};
use crate::{lattice, AnalysisDomain, GenKill, GenKillAnalysis};

mod borrowed_locals;
mod init_locals;
mod liveness;
mod storage_liveness;

pub use self::borrowed_locals::borrowed_locals;
pub use self::borrowed_locals::MaybeBorrowedLocals;
pub use self::init_locals::MaybeInitializedLocals;
pub use self::liveness::MaybeLiveLocals;
pub use self::liveness::MaybeTransitiveLiveLocals;
pub use self::storage_liveness::{MaybeRequiresStorage, MaybeStorageLive};
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_mir_transform/src/dead_store_elimination.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS
for Location { block, statement_index } in patch {
bbs[block].statements[statement_index].make_nop();
}

crate::simplify::SimplifyLocals.run_pass(tcx, body)
}

pub struct DeadStoreElimination;
Expand Down
48 changes: 10 additions & 38 deletions compiler/rustc_mir_transform/src/deduce_param_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,16 @@ impl<'tcx> Visitor<'tcx> for DeduceReadOnly {

if let TerminatorKind::Call { ref args, .. } = terminator.kind {
for arg in args {
if let Operand::Move(_) = *arg {
// ArgumentChecker panics if a direct move of an argument from a caller to a
// callee was detected.
//
// If, in the future, MIR optimizations cause arguments to be moved directly
// from callers to callees, change the panic to instead add the argument in
// question to `mutating_uses`.
ArgumentChecker::new(self.mutable_args.domain_size())
.visit_operand(arg, location)
if let Operand::Move(place) = *arg {
let local = place.local;
if place.is_indirect()
|| local == RETURN_PLACE
|| local.index() > self.mutable_args.domain_size()
{
continue;
}

self.mutable_args.insert(local.index() - 1);
}
}
};
Expand All @@ -127,35 +128,6 @@ impl<'tcx> Visitor<'tcx> for DeduceReadOnly {
}
}

/// A visitor that simply panics if a direct move of an argument from a caller to a callee was
/// detected.
struct ArgumentChecker {
/// The number of arguments to the calling function.
arg_count: usize,
}

impl ArgumentChecker {
/// Creates a new ArgumentChecker.
fn new(arg_count: usize) -> Self {
Self { arg_count }
}
}

impl<'tcx> Visitor<'tcx> for ArgumentChecker {
fn visit_local(&mut self, local: Local, context: PlaceContext, _: Location) {
// Check to make sure that, if this local is an argument, we didn't move directly from it.
if matches!(context, PlaceContext::NonMutatingUse(NonMutatingUseContext::Move))
&& local != RETURN_PLACE
&& local.index() <= self.arg_count
{
// If, in the future, MIR optimizations cause arguments to be moved directly from
// callers to callees, change this panic to instead add the argument in question to
// `mutating_uses`.
panic!("Detected a direct move from a caller's argument to a callee's argument!")
}
}
}

/// Returns true if values of a given type will never be passed indirectly, regardless of ABI.
fn type_will_always_be_passed_directly<'tcx>(ty: Ty<'tcx>) -> bool {
matches!(
Expand Down
Loading

0 comments on commit 0e9eee6

Please sign in to comment.