Skip to content

More improvents to dest prop #95700

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

Closed
wants to merge 3 commits into from
Closed

More improvents to dest prop #95700

Show file tree
Hide file tree
Changes from 2 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
114 changes: 47 additions & 67 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 Down Expand Up @@ -368,6 +368,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 +387,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 +396,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 +439,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 +494,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 +539,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 +669,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