Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

More improvents to dest prop #95700

Closed
wants to merge 3 commits into from
Closed

More improvents to dest prop #95700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9c410b
Stop using initialization analysis in dest prop
JakobDegen Apr 2, 2022
d92e0cc
Catch more overlapping assignments in dest prop
JakobDegen Apr 5, 2022
1862d19
Document work needed for dest prop
JakobDegen Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 68 additions & 71 deletions compiler/rustc_mir_transform/src/dest_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,14 @@ use rustc_index::{
bit_set::{BitMatrix, BitSet},
vec::IndexVec,
};
use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
use rustc_middle::mir::visit::{MutVisitor, NonMutatingUseContext, PlaceContext, Visitor};
use rustc_middle::mir::{dump_mir, PassWhere};
use rustc_middle::mir::{
traversal, Body, InlineAsmOperand, Local, LocalKind, Location, Operand, Place, PlaceElem,
Rvalue, Statement, StatementKind, Terminator, TerminatorKind,
traversal, BasicBlock, Body, InlineAsmOperand, Local, LocalKind, Location, Operand, Place,
PlaceElem, Rvalue, Statement, StatementKind, Terminator, TerminatorKind,
};
use rustc_middle::ty::TyCtxt;
use rustc_mir_dataflow::impls::{MaybeInitializedLocals, MaybeLiveLocals};
use rustc_mir_dataflow::impls::MaybeLiveLocals;
use rustc_mir_dataflow::Analysis;

// Empirical measurements have resulted in some observations:
Expand All @@ -118,10 +118,27 @@ pub struct DestinationPropagation;

impl<'tcx> MirPass<'tcx> for DestinationPropagation {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
// FIXME(#79191, #82678): This is unsound.
//
// Only run at mir-opt-level=3 or higher for now (we don't fix up debuginfo and remove
// storage statements at the moment).
// FIXME(#79191, #82678):
// - Dead store elimination (soundness critical, definitely the cause of #79191). The gist
// of this optimization is that two distinct "variables" can reuse a local if they have
// no need to use that local at the same time. We determine that this is the case by
// doing a liveness analysis on the variables; however, that is only sufficient if we
// then actually go and guarantee that when the variable is dead, it does not go
// overwriting the current value in the local. This is ensured by a DSE pass. It is
// JakobDegen's opinion that this should not be implemented as a separate `MirPass`, but
// rather as a part of this one, because it is critical for soundness that the DSE be
// done with the same (or at least strictly stronger) analysis facts as are used for the
// rest of the optimization.
// - Bad handling of unreachable blocks (soundness critical, possibly the cause of #82678).
// The current MIR semantics define overlapping argument and return places for function
// calls to be statically disallowed, not just UB. This means it does not suffice to
// consider only reachable basic blocks in `Conflicts::Build`. There are a lot of ways to
// address this.
// - Double check handling of intra-statement conficts (soundness critical). This is
// currently somewhat ad-hoc, and probably can't be done correctly without specifying
// more MIR semantics.
// - Fix debug info (non-soundness critical)
// - Fix storage statements (non-soundness critical)
sess.opts.debugging_opts.unsound_mir_opts && sess.mir_opt_level() >= 3
}

Expand Down Expand Up @@ -368,6 +385,12 @@ struct Conflicts<'a> {
}

impl<'a> Conflicts<'a> {
/// Build the conflicts table from the MIR body
///
/// This will mark two locals as conflicting, if they are ever live at the same time with a
/// single exception: Assign statements with `use` rvalues. If the optimization merges two
/// places, then it must promise to remove all assignments between those places in either
/// direction.
fn build<'tcx>(
tcx: TyCtxt<'tcx>,
body: &'_ Body<'tcx>,
Expand All @@ -381,10 +404,6 @@ impl<'a> Conflicts<'a> {
body.local_decls.len(),
);

let mut init = MaybeInitializedLocals
.into_engine(tcx, body)
.iterate_to_fixpoint()
.into_results_cursor(body);
let mut live =
MaybeLiveLocals.into_engine(tcx, body).iterate_to_fixpoint().into_results_cursor(body);

Expand All @@ -394,38 +413,27 @@ impl<'a> Conflicts<'a> {

match pass_where {
PassWhere::BeforeLocation(loc) if reachable.contains(loc.block) => {
init.seek_before_primary_effect(loc);
live.seek_after_primary_effect(loc);

writeln!(w, " // init: {:?}", init.get())?;
writeln!(w, " // live: {:?}", live.get())?;
}
PassWhere::AfterTerminator(bb) if reachable.contains(bb) => {
let loc = body.terminator_loc(bb);
init.seek_after_primary_effect(loc);
live.seek_before_primary_effect(loc);

writeln!(w, " // init: {:?}", init.get())?;
writeln!(w, " // live: {:?}", live.get())?;
}

PassWhere::BeforeBlock(bb) if reachable.contains(bb) => {
init.seek_to_block_start(bb);
live.seek_to_block_start(bb);

writeln!(w, " // init: {:?}", init.get())?;
writeln!(w, " // live: {:?}", live.get())?;
}

PassWhere::BeforeCFG | PassWhere::AfterCFG | PassWhere::AfterLocation(_) => {}

PassWhere::BeforeLocation(_) | PassWhere::AfterTerminator(_) => {
writeln!(w, " // init: <unreachable>")?;
writeln!(w, " // live: <unreachable>")?;
}

PassWhere::BeforeBlock(_) => {
writeln!(w, " // init: <unreachable>")?;
writeln!(w, " // live: <unreachable>")?;
}
}
Expand All @@ -448,55 +456,34 @@ impl<'a> Conflicts<'a> {
},
};

let mut live_and_init_locals = Vec::new();

// Visit only reachable basic blocks. The exact order is not important.
for (block, data) in traversal::preorder(body) {
// We need to observe the dataflow state *before* all possible locations (statement or
// terminator) in each basic block, and then observe the state *after* the terminator
// effect is applied. As long as neither `init` nor `borrowed` has a "before" effect,
// we will observe all possible dataflow states.

// Since liveness is a backwards analysis, we need to walk the results backwards. To do
// that, we first collect in the `MaybeInitializedLocals` results in a forwards
// traversal.

live_and_init_locals.resize_with(data.statements.len() + 1, || {
BitSet::new_empty(body.local_decls.len())
});

// First, go forwards for `MaybeInitializedLocals` and apply intra-statement/terminator
// conflicts.
for (i, statement) in data.statements.iter().enumerate() {
// First, apply intra-statement/terminator conflicts.
for statement in data.statements.iter() {
this.record_statement_conflicts(statement);

let loc = Location { block, statement_index: i };
init.seek_before_primary_effect(loc);

live_and_init_locals[i].clone_from(init.get());
}

this.record_terminator_conflicts(data.terminator());
let term_loc = Location { block, statement_index: data.statements.len() };
init.seek_before_primary_effect(term_loc);
live_and_init_locals[term_loc.statement_index].clone_from(init.get());

// Now, go backwards and union with the liveness results.
// Now, apply liveness results.
for statement_index in (0..=data.statements.len()).rev() {
let loc = Location { block, statement_index };
live.seek_after_primary_effect(loc);

live_and_init_locals[statement_index].intersect(live.get());
let mut live_locals = live.get().clone();

trace!("record conflicts at {:?}", loc);

this.record_dataflow_conflicts(&mut live_and_init_locals[statement_index]);
this.record_dataflow_conflicts(&mut live_locals);
}

init.seek_to_block_end(block);
live.seek_to_block_end(block);
let mut conflicts = init.get().clone();
conflicts.intersect(live.get());
let mut conflicts = live.get().clone();
trace!("record conflicts at end of {:?}", block);

this.record_dataflow_conflicts(&mut conflicts);
Expand Down Expand Up @@ -524,10 +511,36 @@ impl<'a> Conflicts<'a> {
/// and must not be merged.
fn record_statement_conflicts(&mut self, stmt: &Statement<'_>) {
match &stmt.kind {
// While the left and right sides of an assignment must not overlap, we do not mark
// conflicts here as that would make this optimization useless. When we optimize, we
// eliminate the resulting self-assignments automatically.
StatementKind::Assign(_) => {}
StatementKind::Assign(v) => {
let p = v.0;
let rvalue = &v.1;
if p.is_indirect() {
return;
}
let lhs_local = p.local;

struct RvalueVisitor<'a, 'b>(&'a mut Conflicts<'b>, Local);

impl<'a, 'b, 'tcx> Visitor<'tcx> for RvalueVisitor<'a, 'b> {
fn visit_local(&mut self, local: &Local, _: PlaceContext, _: Location) {
self.0.record_local_conflict(self.1, *local, "intra-assignment");
}
}

let mut v = RvalueVisitor(self, lhs_local);
let location = Location { block: BasicBlock::MAX, statement_index: 0 };
if let Rvalue::Use(op) = rvalue {
if let Some(rhs_place) = op.place() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave a comment that this explicitly skips the place and only looks for index places in the projection

v.visit_projection(
rhs_place.as_ref(),
PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy),
location,
);
}
} else {
v.visit_rvalue(rvalue, location);
}
}

StatementKind::SetDiscriminant { .. }
| StatementKind::StorageLive(..)
Expand All @@ -543,23 +556,6 @@ impl<'a> Conflicts<'a> {

fn record_terminator_conflicts(&mut self, term: &Terminator<'_>) {
match &term.kind {
TerminatorKind::DropAndReplace {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we need this anymore? Because we run so late this variant doesn't occur? Should probably group the ones that can't happen into a => bug!() arm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DropAndReplace gets removed during drop elaboration.

place: dropped_place,
value,
target: _,
unwind: _,
} => {
if let Some(place) = value.place()
&& !place.is_indirect()
&& !dropped_place.is_indirect()
{
self.record_local_conflict(
place.local,
dropped_place.local,
"DropAndReplace operand overlap",
);
}
}
TerminatorKind::Yield { value, resume: _, resume_arg, drop: _ } => {
if let Some(place) = value.place() {
if !place.is_indirect() && !resume_arg.is_indirect() {
Expand Down Expand Up @@ -690,6 +686,7 @@ impl<'a> Conflicts<'a> {
}

TerminatorKind::Goto { .. }
| TerminatorKind::DropAndReplace { .. }
| TerminatorKind::Call { destination: None, .. }
| TerminatorKind::SwitchInt { .. }
| TerminatorKind::Resume
Expand Down
18 changes: 6 additions & 12 deletions src/test/mir-opt/dest-prop/union.main.DestinationPropagation.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,23 @@
}

bb0: {
- StorageLive(_1); // scope 0 at $DIR/union.rs:13:9: 13:11
- StorageLive(_2); // scope 0 at $DIR/union.rs:13:23: 13:28
- _2 = val() -> bb1; // scope 0 at $DIR/union.rs:13:23: 13:28
+ nop; // scope 0 at $DIR/union.rs:13:9: 13:11
+ nop; // scope 0 at $DIR/union.rs:13:23: 13:28
+ (_1.0: u32) = val() -> bb1; // scope 0 at $DIR/union.rs:13:23: 13:28
StorageLive(_1); // scope 0 at $DIR/union.rs:13:9: 13:11
StorageLive(_2); // scope 0 at $DIR/union.rs:13:23: 13:28
_2 = val() -> bb1; // scope 0 at $DIR/union.rs:13:23: 13:28
// mir::Constant
// + span: $DIR/union.rs:13:23: 13:26
// + literal: Const { ty: fn() -> u32 {val}, val: Value(Scalar(<ZST>)) }
}

bb1: {
- (_1.0: u32) = move _2; // scope 0 at $DIR/union.rs:13:14: 13:30
- StorageDead(_2); // scope 0 at $DIR/union.rs:13:29: 13:30
+ nop; // scope 0 at $DIR/union.rs:13:14: 13:30
+ nop; // scope 0 at $DIR/union.rs:13:29: 13:30
(_1.0: u32) = move _2; // scope 0 at $DIR/union.rs:13:14: 13:30
StorageDead(_2); // scope 0 at $DIR/union.rs:13:29: 13:30
StorageLive(_3); // scope 1 at $DIR/union.rs:15:5: 15:27
StorageLive(_4); // scope 1 at $DIR/union.rs:15:10: 15:26
_4 = (_1.0: u32); // scope 2 at $DIR/union.rs:15:19: 15:24
StorageDead(_4); // scope 1 at $DIR/union.rs:15:26: 15:27
StorageDead(_3); // scope 1 at $DIR/union.rs:15:27: 15:28
- StorageDead(_1); // scope 0 at $DIR/union.rs:16:1: 16:2
+ nop; // scope 0 at $DIR/union.rs:16:1: 16:2
StorageDead(_1); // scope 0 at $DIR/union.rs:16:1: 16:2
return; // scope 0 at $DIR/union.rs:16:2: 16:2
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/test/mir-opt/dest-prop/union.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@ fn main() {

drop(unsafe { un.us });
}

// FIXME(JakobDegen): This example is currently broken; it needs a more precise liveness analysis in
// order to be fixed.
21 changes: 7 additions & 14 deletions ..._branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,16 @@

bb0: {
- StorageLive(_3); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:8: 27:6
- StorageLive(_4); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:14: 21:24
- StorageLive(_5); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:15: 21:16
- _5 = _1; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:15: 21:16
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:8: 27:6
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:14: 21:24
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:15: 21:16
+ (_4.0: &ViewportPercentageLength) = _1; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:15: 21:16
StorageLive(_4); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:14: 21:24
StorageLive(_5); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:15: 21:16
_5 = _1; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:15: 21:16
StorageLive(_6); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:18: 21:23
_6 = _2; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:18: 21:23
- (_4.0: &ViewportPercentageLength) = move _5; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:14: 21:24
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:14: 21:24
(_4.0: &ViewportPercentageLength) = move _5; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:14: 21:24
(_4.1: &ViewportPercentageLength) = move _6; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:14: 21:24
StorageDead(_6); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:23: 21:24
- StorageDead(_5); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:23: 21:24
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:23: 21:24
StorageDead(_5); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:23: 21:24
_11 = discriminant((*(_4.0: &ViewportPercentageLength))); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:14: 21:24
- switchInt(move _11) -> [0_isize: bb1, 1_isize: bb3, 2_isize: bb4, 3_isize: bb5, otherwise: bb11]; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:8: 21:24
+ StorageLive(_34); // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:8: 21:24
Expand All @@ -102,9 +97,8 @@
discriminant(_0) = 1; // scope 0 at $DIR/early_otherwise_branch_68867.rs:26:21: 26:28
StorageDead(_33); // scope 0 at $DIR/early_otherwise_branch_68867.rs:26:27: 26:28
- StorageDead(_3); // scope 0 at $DIR/early_otherwise_branch_68867.rs:27:6: 27:7
- StorageDead(_4); // scope 0 at $DIR/early_otherwise_branch_68867.rs:28:1: 28:2
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:27:6: 27:7
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:28:1: 28:2
StorageDead(_4); // scope 0 at $DIR/early_otherwise_branch_68867.rs:28:1: 28:2
return; // scope 0 at $DIR/early_otherwise_branch_68867.rs:28:2: 28:2
}

Expand Down Expand Up @@ -287,9 +281,8 @@
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:5: 27:7
discriminant(_0) = 0; // scope 0 at $DIR/early_otherwise_branch_68867.rs:21:5: 27:7
- StorageDead(_3); // scope 0 at $DIR/early_otherwise_branch_68867.rs:27:6: 27:7
- StorageDead(_4); // scope 0 at $DIR/early_otherwise_branch_68867.rs:28:1: 28:2
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:27:6: 27:7
+ nop; // scope 0 at $DIR/early_otherwise_branch_68867.rs:28:1: 28:2
StorageDead(_4); // scope 0 at $DIR/early_otherwise_branch_68867.rs:28:1: 28:2
return; // scope 0 at $DIR/early_otherwise_branch_68867.rs:28:2: 28:2
}

Expand Down