diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs index 5d0b58e9c5360..48ce1b72ea74f 100644 --- a/compiler/rustc_mir_transform/src/dest_prop.rs +++ b/compiler/rustc_mir_transform/src/dest_prop.rs @@ -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: @@ -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 } @@ -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>, @@ -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); @@ -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: ")?; writeln!(w, " // live: ")?; } PassWhere::BeforeBlock(_) => { - writeln!(w, " // init: ")?; writeln!(w, " // live: ")?; } } @@ -448,8 +456,6 @@ 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 @@ -457,46 +463,27 @@ impl<'a> Conflicts<'a> { // 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); @@ -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() { + v.visit_projection( + rhs_place.as_ref(), + PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy), + location, + ); + } + } else { + v.visit_rvalue(rvalue, location); + } + } StatementKind::SetDiscriminant { .. } | StatementKind::StorageLive(..) @@ -543,23 +556,6 @@ impl<'a> Conflicts<'a> { fn record_terminator_conflicts(&mut self, term: &Terminator<'_>) { match &term.kind { - TerminatorKind::DropAndReplace { - 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() { @@ -690,6 +686,7 @@ impl<'a> Conflicts<'a> { } TerminatorKind::Goto { .. } + | TerminatorKind::DropAndReplace { .. } | TerminatorKind::Call { destination: None, .. } | TerminatorKind::SwitchInt { .. } | TerminatorKind::Resume diff --git a/src/test/mir-opt/dest-prop/union.main.DestinationPropagation.diff b/src/test/mir-opt/dest-prop/union.main.DestinationPropagation.diff index ab60a7fc62f21..46916d000e0f0 100644 --- a/src/test/mir-opt/dest-prop/union.main.DestinationPropagation.diff +++ b/src/test/mir-opt/dest-prop/union.main.DestinationPropagation.diff @@ -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()) } } 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 } } diff --git a/src/test/mir-opt/dest-prop/union.rs b/src/test/mir-opt/dest-prop/union.rs index 68c834dfbbf27..2dc3f79ca7e1e 100644 --- a/src/test/mir-opt/dest-prop/union.rs +++ b/src/test/mir-opt/dest-prop/union.rs @@ -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. diff --git a/src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff b/src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff index db6794db29819..d342b7b994dbe 100644 --- a/src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff +++ b/src/test/mir-opt/early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.before-SimplifyConstCondition-final.after.diff @@ -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 @@ -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 } @@ -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 }