From 4a75995fbd0c5a342fe0556eba080e1ae7fe0802 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 8 Feb 2023 20:30:44 +0000 Subject: [PATCH 1/3] Move state fixup into a different method. --- compiler/rustc_mir_transform/src/const_prop.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 33ee90ffc119d..54899fd4db49d 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -13,11 +13,7 @@ use rustc_index::vec::IndexVec; use rustc_middle::mir::visit::{ MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor, }; -use rustc_middle::mir::{ - BasicBlock, BinOp, Body, Constant, ConstantKind, Local, LocalDecl, LocalKind, Location, - Operand, Place, Rvalue, SourceInfo, Statement, StatementKind, Terminator, TerminatorKind, - RETURN_PLACE, -}; +use rustc_middle::mir::*; use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout}; use rustc_middle::ty::InternalSubsts; use rustc_middle::ty::{self, ConstKind, Instance, ParamEnv, Ty, TyCtxt, TypeVisitable}; @@ -1008,7 +1004,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { let source_info = terminator.source_info; self.source_info = Some(source_info); self.super_terminator(terminator, location); - // Do NOT early return in this function, it does some crucial fixup of the state at the end! + match &mut terminator.kind { TerminatorKind::Assert { expected, ref mut cond, .. } => { if let Some(ref value) = self.eval_operand(&cond) @@ -1050,6 +1046,10 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { // gated on `mir_opt_level=3`. TerminatorKind::Call { .. } => {} } + } + + fn visit_basic_block_data(&mut self, block: BasicBlock, data: &mut BasicBlockData<'tcx>) { + self.super_basic_block_data(block, data); // We remove all Locals which are restricted in propagation to their containing blocks and // which were modified in the current block. From d0934f14c7fd95c0517c98a2c0dc549ec40b03e7 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 8 Feb 2023 20:32:28 +0000 Subject: [PATCH 2/3] Merge if-let and match. --- .../rustc_mir_transform/src/const_prop.rs | 130 +++++++++--------- 1 file changed, 63 insertions(+), 67 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 54899fd4db49d..2de8079a4dfb4 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -917,84 +917,80 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { trace!("visit_statement: {:?}", statement); let source_info = statement.source_info; self.source_info = Some(source_info); - if let StatementKind::Assign(box (place, ref mut rval)) = statement.kind { - let can_const_prop = self.ecx.machine.can_const_prop[place.local]; - if let Some(()) = self.const_prop(rval, place) { - // This will return None if the above `const_prop` invocation only "wrote" a - // type whose creation requires no write. E.g. a generator whose initial state - // consists solely of uninitialized memory (so it doesn't capture any locals). - if let Some(ref value) = self.get_const(place) && self.should_const_prop(value) { - trace!("replacing {:?} with {:?}", rval, value); - self.replace_with_const(rval, value, source_info); - if can_const_prop == ConstPropMode::FullConstProp - || can_const_prop == ConstPropMode::OnlyInsideOwnBlock - { - trace!("propagated into {:?}", place); + match statement.kind { + StatementKind::Assign(box (place, ref mut rval)) => { + let can_const_prop = self.ecx.machine.can_const_prop[place.local]; + if let Some(()) = self.const_prop(rval, place) { + // This will return None if the above `const_prop` invocation only "wrote" a + // type whose creation requires no write. E.g. a generator whose initial state + // consists solely of uninitialized memory (so it doesn't capture any locals). + if let Some(ref value) = self.get_const(place) && self.should_const_prop(value) { + trace!("replacing {:?} with {:?}", rval, value); + self.replace_with_const(rval, value, source_info); + if can_const_prop == ConstPropMode::FullConstProp + || can_const_prop == ConstPropMode::OnlyInsideOwnBlock + { + trace!("propagated into {:?}", place); + } } - } - match can_const_prop { - ConstPropMode::OnlyInsideOwnBlock => { - trace!( - "found local restricted to its block. \ + match can_const_prop { + ConstPropMode::OnlyInsideOwnBlock => { + trace!( + "found local restricted to its block. \ Will remove it from const-prop after block is finished. Local: {:?}", - place.local - ); - } - ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { - trace!("can't propagate into {:?}", place); - if place.local != RETURN_PLACE { - Self::remove_const(&mut self.ecx, place.local); + place.local + ); } - } - ConstPropMode::FullConstProp => {} - } - } else { - // Const prop failed, so erase the destination, ensuring that whatever happens - // from here on, does not know about the previous value. - // This is important in case we have - // ```rust - // let mut x = 42; - // x = SOME_MUTABLE_STATIC; - // // x must now be uninit - // ``` - // FIXME: we overzealously erase the entire local, because that's easier to - // implement. - trace!( - "propagation into {:?} failed. - Nuking the entire site from orbit, it's the only way to be sure", - place, - ); - Self::remove_const(&mut self.ecx, place.local); - } - } else { - match statement.kind { - StatementKind::SetDiscriminant { ref place, .. } => { - match self.ecx.machine.can_const_prop[place.local] { - ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => { - if self.use_ecx(|this| this.ecx.statement(statement)).is_some() { - trace!("propped discriminant into {:?}", place); - } else { + ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { + trace!("can't propagate into {:?}", place); + if place.local != RETURN_PLACE { Self::remove_const(&mut self.ecx, place.local); } } - ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { - Self::remove_const(&mut self.ecx, place.local); - } + ConstPropMode::FullConstProp => {} } + } else { + // Const prop failed, so erase the destination, ensuring that whatever happens + // from here on, does not know about the previous value. + // This is important in case we have + // ```rust + // let mut x = 42; + // x = SOME_MUTABLE_STATIC; + // // x must now be uninit + // ``` + // FIXME: we overzealously erase the entire local, because that's easier to + // implement. + trace!( + "propagation into {:?} failed. + Nuking the entire site from orbit, it's the only way to be sure", + place, + ); + Self::remove_const(&mut self.ecx, place.local); } - StatementKind::StorageLive(local) | StatementKind::StorageDead(local) => { - let frame = self.ecx.frame_mut(); - frame.locals[local].value = - if let StatementKind::StorageLive(_) = statement.kind { - LocalValue::Live(interpret::Operand::Immediate( - interpret::Immediate::Uninit, - )) + } + StatementKind::SetDiscriminant { ref place, .. } => { + match self.ecx.machine.can_const_prop[place.local] { + ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => { + if self.use_ecx(|this| this.ecx.statement(statement)).is_some() { + trace!("propped discriminant into {:?}", place); } else { - LocalValue::Dead - }; + Self::remove_const(&mut self.ecx, place.local); + } + } + ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { + Self::remove_const(&mut self.ecx, place.local); + } } - _ => {} } + StatementKind::StorageLive(local) | StatementKind::StorageDead(local) => { + let frame = self.ecx.frame_mut(); + frame.locals[local].value = if let StatementKind::StorageLive(_) = statement.kind { + LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit)) + } else { + LocalValue::Dead + }; + } + _ => {} } self.super_statement(statement, location); From f02d6c45e1117ef1d4e16f07b52ef6fe6f809ae4 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Fri, 10 Feb 2023 18:33:41 +0000 Subject: [PATCH 3/3] Remove use_ecx. --- .../rustc_mir_transform/src/const_prop.rs | 97 +++++++------------ 1 file changed, 37 insertions(+), 60 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 2de8079a4dfb4..cff3da720fc5c 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -452,27 +452,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { }; } - fn use_ecx(&mut self, f: F) -> Option - where - F: FnOnce(&mut Self) -> InterpResult<'tcx, T>, - { - match f(self) { - Ok(val) => Some(val), - Err(error) => { - trace!("InterpCx operation failed: {:?}", error); - // Some errors shouldn't come up because creating them causes - // an allocation, which we should avoid. When that happens, - // dedicated error variants should be introduced instead. - assert!( - !error.kind().formatted_string(), - "const-prop encountered formatting error: {}", - error - ); - None - } - } - } - /// Returns the value, if any, of evaluating `c`. fn eval_constant(&mut self, c: &Constant<'tcx>) -> Option> { // FIXME we need to revisit this for #67176 @@ -487,7 +466,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { /// Returns the value, if any, of evaluating `place`. fn eval_place(&mut self, place: Place<'tcx>) -> Option> { trace!("eval_place(place={:?})", place); - self.use_ecx(|this| this.ecx.eval_place_to_op(place, None)) + self.ecx.eval_place_to_op(place, None).ok() } /// Returns the value, if any, of evaluating `op`. Calls upon `eval_constant` @@ -591,35 +570,37 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { rvalue: &Rvalue<'tcx>, place: Place<'tcx>, ) -> Option<()> { - self.use_ecx(|this| match rvalue { + match rvalue { Rvalue::BinaryOp(op, box (left, right)) | Rvalue::CheckedBinaryOp(op, box (left, right)) => { - let l = this.ecx.eval_operand(left, None).and_then(|x| this.ecx.read_immediate(&x)); + let l = self.ecx.eval_operand(left, None).and_then(|x| self.ecx.read_immediate(&x)); let r = - this.ecx.eval_operand(right, None).and_then(|x| this.ecx.read_immediate(&x)); + self.ecx.eval_operand(right, None).and_then(|x| self.ecx.read_immediate(&x)); let const_arg = match (l, r) { (Ok(x), Err(_)) | (Err(_), Ok(x)) => x, // exactly one side is known - (Err(e), Err(_)) => return Err(e), // neither side is known - (Ok(_), Ok(_)) => return this.ecx.eval_rvalue_into_place(rvalue, place), // both sides are known + (Err(_), Err(_)) => return None, // neither side is known + (Ok(_), Ok(_)) => return self.ecx.eval_rvalue_into_place(rvalue, place).ok(), // both sides are known }; if !matches!(const_arg.layout.abi, abi::Abi::Scalar(..)) { // We cannot handle Scalar Pair stuff. // No point in calling `eval_rvalue_into_place`, since only one side is known - throw_machine_stop_str!("cannot optimize this") + return None; } - let arg_value = const_arg.to_scalar().to_bits(const_arg.layout.size)?; - let dest = this.ecx.eval_place(place)?; + let arg_value = const_arg.to_scalar().to_bits(const_arg.layout.size).ok()?; + let dest = self.ecx.eval_place(place).ok()?; match op { - BinOp::BitAnd if arg_value == 0 => this.ecx.write_immediate(*const_arg, &dest), + BinOp::BitAnd if arg_value == 0 => { + self.ecx.write_immediate(*const_arg, &dest).ok() + } BinOp::BitOr if arg_value == const_arg.layout.size.truncate(u128::MAX) || (const_arg.layout.ty.is_bool() && arg_value == 1) => { - this.ecx.write_immediate(*const_arg, &dest) + self.ecx.write_immediate(*const_arg, &dest).ok() } BinOp::Mul if const_arg.layout.ty.is_integral() && arg_value == 0 => { if let Rvalue::CheckedBinaryOp(_, _) = rvalue { @@ -627,16 +608,16 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { const_arg.to_scalar(), Scalar::from_bool(false), ); - this.ecx.write_immediate(val, &dest) + self.ecx.write_immediate(val, &dest).ok() } else { - this.ecx.write_immediate(*const_arg, &dest) + self.ecx.write_immediate(*const_arg, &dest).ok() } } - _ => throw_machine_stop_str!("cannot optimize this"), + _ => None, } } - _ => this.ecx.eval_rvalue_into_place(rvalue, place), - }) + _ => self.ecx.eval_rvalue_into_place(rvalue, place).ok(), + } } /// Creates a new `Operand::Constant` from a `Scalar` value @@ -678,7 +659,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } // FIXME> figure out what to do when read_immediate_raw fails - let imm = self.use_ecx(|this| this.ecx.read_immediate_raw(value)); + let imm = self.ecx.read_immediate_raw(value).ok(); if let Some(Right(imm)) = imm { match *imm { @@ -698,25 +679,23 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { if let ty::Tuple(types) = ty.kind() { // Only do it if tuple is also a pair with two scalars if let [ty1, ty2] = types[..] { - let alloc = self.use_ecx(|this| { - let ty_is_scalar = |ty| { - this.ecx.layout_of(ty).ok().map(|layout| layout.abi.is_scalar()) - == Some(true) - }; - if ty_is_scalar(ty1) && ty_is_scalar(ty2) { - let alloc = this - .ecx - .intern_with_temp_alloc(value.layout, |ecx, dest| { - ecx.write_immediate(*imm, dest) - }) - .unwrap(); - Ok(Some(alloc)) - } else { - Ok(None) - } - }); - - if let Some(Some(alloc)) = alloc { + let ty_is_scalar = |ty| { + self.ecx.layout_of(ty).ok().map(|layout| layout.abi.is_scalar()) + == Some(true) + }; + let alloc = if ty_is_scalar(ty1) && ty_is_scalar(ty2) { + let alloc = self + .ecx + .intern_with_temp_alloc(value.layout, |ecx, dest| { + ecx.write_immediate(*imm, dest) + }) + .unwrap(); + Some(alloc) + } else { + None + }; + + if let Some(alloc) = alloc { // Assign entire constant in a single statement. // We can't use aggregates, as we run after the aggregate-lowering `MirPhase`. let const_val = ConstValue::ByRef { alloc, offset: Size::ZERO }; @@ -971,7 +950,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { StatementKind::SetDiscriminant { ref place, .. } => { match self.ecx.machine.can_const_prop[place.local] { ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => { - if self.use_ecx(|this| this.ecx.statement(statement)).is_some() { + if self.ecx.statement(statement).is_ok() { trace!("propped discriminant into {:?}", place); } else { Self::remove_const(&mut self.ecx, place.local); @@ -1004,8 +983,6 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { match &mut terminator.kind { TerminatorKind::Assert { expected, ref mut cond, .. } => { if let Some(ref value) = self.eval_operand(&cond) - // FIXME should be used use_ecx rather than a local match... but we have - // quite a few of these read_scalar/read_immediate that need fixing. && let Ok(value_const) = self.ecx.read_scalar(&value) && self.should_const_prop(value) {