From 9a56933e8cace1e2169a0ab60854ed41e12f81e1 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 14:31:43 +0000 Subject: [PATCH 01/13] Create visit_block_data for const-prop-lint. --- compiler/rustc_mir_transform/src/const_prop_lint.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 6c1980ff3ad9..d042f93009e8 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -14,11 +14,7 @@ use rustc_hir::HirId; use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; use rustc_middle::mir::visit::Visitor; -use rustc_middle::mir::{ - AssertKind, BinOp, Body, Constant, Local, LocalDecl, Location, Operand, Place, Rvalue, - SourceInfo, SourceScope, SourceScopeData, Statement, StatementKind, Terminator, TerminatorKind, - UnOp, RETURN_PLACE, -}; +use rustc_middle::mir::*; use rustc_middle::ty::layout::{LayoutError, LayoutOf, LayoutOfHelpers, TyAndLayout}; use rustc_middle::ty::InternalSubsts; use rustc_middle::ty::{ @@ -695,6 +691,10 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { | TerminatorKind::Call { .. } | TerminatorKind::InlineAsm { .. } => {} } + } + + fn visit_basic_block_data(&mut self, block: BasicBlock, data: &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 2247cd664368acc07529a498388ab78eed93c431 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 14:41:13 +0000 Subject: [PATCH 02/13] Simplify visit_statement. --- .../rustc_mir_transform/src/const_prop.rs | 13 +- .../src/const_prop_lint.rs | 113 +++++++++--------- 2 files changed, 61 insertions(+), 65 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 6b2eefce24d5..9a3ccc2f7e4d 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -961,13 +961,14 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { } } } - StatementKind::StorageLive(local) | StatementKind::StorageDead(local) => { + StatementKind::StorageLive(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 - }; + frame.locals[local].value = + LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit)); + } + StatementKind::StorageDead(local) => { + let frame = self.ecx.frame_mut(); + frame.locals[local].value = LocalValue::Dead; } _ => {} } diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index d042f93009e8..030d79ac22de 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -522,75 +522,70 @@ impl<'tcx> Visitor<'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 rval)) = statement.kind { - let can_const_prop = self.ecx.machine.can_const_prop[place.local]; - if let Some(()) = self.const_prop(rval, source_info, place) { - match can_const_prop { - ConstPropMode::OnlyInsideOwnBlock => { - trace!( - "found local restricted to its block. \ + match statement.kind { + StatementKind::Assign(box (place, ref rval)) => { + let can_const_prop = self.ecx.machine.can_const_prop[place.local]; + if let Some(()) = self.const_prop(rval, source_info, place) { + 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(source_info, |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(source_info, |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) => { + let frame = self.ecx.frame_mut(); + frame.locals[local].value = + LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit)); + } + StatementKind::StorageDead(local) => { + let frame = self.ecx.frame_mut(); + frame.locals[local].value = LocalValue::Dead; + } + _ => {} } self.super_statement(statement, location); From 0e64ce7c5e6c15655ba99c1cbd7f3fe4f6125eb8 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sun, 5 Mar 2023 21:37:39 +0000 Subject: [PATCH 03/13] Do not track span in ConstProp. --- .../rustc_mir_transform/src/const_prop.rs | 39 +++++-------------- .../discriminant.main.ConstProp.64bit.diff | 2 +- .../invalid_constant.main.ConstProp.diff | 4 +- ...float_to_exponential_common.ConstProp.diff | 4 +- 4 files changed, 14 insertions(+), 35 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 9a3ccc2f7e4d..afc5f08b2d2b 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -17,7 +17,7 @@ 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, TypeVisitableExt}; -use rustc_span::{def_id::DefId, Span}; +use rustc_span::{def_id::DefId, Span, DUMMY_SP}; use rustc_target::abi::{self, Align, HasDataLayout, Size, TargetDataLayout}; use rustc_target::spec::abi::Abi as CallAbi; use rustc_trait_selection::traits; @@ -328,9 +328,6 @@ struct ConstPropagator<'mir, 'tcx> { tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>, local_decls: &'mir IndexVec>, - // Because we have `MutVisitor` we can't obtain the `SourceInfo` from a `Location`. So we store - // the last known `SourceInfo` here and just keep revisiting it. - source_info: Option, } impl<'tcx> LayoutOfHelpers<'tcx> for ConstPropagator<'_, 'tcx> { @@ -411,13 +408,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) .expect("failed to push initial stack frame"); - ConstPropagator { - ecx, - tcx, - param_env, - local_decls: &dummy_body.local_decls, - source_info: None, - } + ConstPropagator { ecx, tcx, param_env, local_decls: &dummy_body.local_decls } } fn get_const(&self, place: Place<'tcx>) -> Option> { @@ -495,7 +486,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { *operand = self.operand_from_scalar( scalar, value.layout.ty, - self.source_info.unwrap().span, + DUMMY_SP, ); } } @@ -629,12 +620,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { })) } - fn replace_with_const( - &mut self, - rval: &mut Rvalue<'tcx>, - value: &OpTy<'tcx>, - source_info: SourceInfo, - ) { + fn replace_with_const(&mut self, rval: &mut Rvalue<'tcx>, value: &OpTy<'tcx>) { if let Rvalue::Use(Operand::Constant(c)) = rval { match c.literal { ConstantKind::Ty(c) if matches!(c.kind(), ConstKind::Unevaluated(..)) => {} @@ -664,11 +650,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { if let Some(Right(imm)) = imm { match *imm { interpret::Immediate::Scalar(scalar) => { - *rval = Rvalue::Use(self.operand_from_scalar( - scalar, - value.layout.ty, - source_info.span, - )); + *rval = + Rvalue::Use(self.operand_from_scalar(scalar, value.layout.ty, DUMMY_SP)); } Immediate::ScalarPair(..) => { // Found a value represented as a pair. For now only do const-prop if the type @@ -701,7 +684,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let const_val = ConstValue::ByRef { alloc, offset: Size::ZERO }; let literal = ConstantKind::Val(const_val, ty); *rval = Rvalue::Use(Operand::Constant(Box::new(Constant { - span: source_info.span, + span: DUMMY_SP, user_ty: None, literal, }))); @@ -894,8 +877,6 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Location) { trace!("visit_statement: {:?}", statement); - let source_info = statement.source_info; - self.source_info = Some(source_info); match statement.kind { StatementKind::Assign(box (place, ref mut rval)) => { let can_const_prop = self.ecx.machine.can_const_prop[place.local]; @@ -905,7 +886,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { // 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); + self.replace_with_const(rval, value); if can_const_prop == ConstPropMode::FullConstProp || can_const_prop == ConstPropMode::OnlyInsideOwnBlock { @@ -977,8 +958,6 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { } fn visit_terminator(&mut self, terminator: &mut Terminator<'tcx>, location: Location) { - let source_info = terminator.source_info; - self.source_info = Some(source_info); self.super_terminator(terminator, location); match &mut terminator.kind { @@ -991,7 +970,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { *cond = self.operand_from_scalar( value_const, self.tcx.types.bool, - source_info.span, + DUMMY_SP, ); } } diff --git a/tests/mir-opt/const_prop/discriminant.main.ConstProp.64bit.diff b/tests/mir-opt/const_prop/discriminant.main.ConstProp.64bit.diff index b9a10704be0d..6d8738aa61aa 100644 --- a/tests/mir-opt/const_prop/discriminant.main.ConstProp.64bit.diff +++ b/tests/mir-opt/const_prop/discriminant.main.ConstProp.64bit.diff @@ -22,7 +22,7 @@ - switchInt(move _4) -> [1: bb1, otherwise: bb3]; // scope 2 at $DIR/discriminant.rs:+1:21: +1:31 + _3 = const Option::::Some(true); // scope 2 at $DIR/discriminant.rs:+1:34: +1:44 + // mir::Constant -+ // + span: $DIR/discriminant.rs:12:34: 12:44 ++ // + span: no-location + // + literal: Const { ty: Option, val: Value(Scalar(0x01)) } + _4 = const 1_isize; // scope 2 at $DIR/discriminant.rs:+1:21: +1:31 + switchInt(const 1_isize) -> [1: bb1, otherwise: bb3]; // scope 2 at $DIR/discriminant.rs:+1:21: +1:31 diff --git a/tests/mir-opt/const_prop/invalid_constant.main.ConstProp.diff b/tests/mir-opt/const_prop/invalid_constant.main.ConstProp.diff index 4f056dd85e3f..a38c1de2a783 100644 --- a/tests/mir-opt/const_prop/invalid_constant.main.ConstProp.diff +++ b/tests/mir-opt/const_prop/invalid_constant.main.ConstProp.diff @@ -44,11 +44,11 @@ - _3 = [move _4]; // scope 1 at $DIR/invalid_constant.rs:+13:24: +13:60 + _4 = const Scalar(0x00000004): E; // scope 4 at $DIR/invalid_constant.rs:+13:34: +13:57 + // mir::Constant -+ // + span: $DIR/invalid_constant.rs:28:34: 28:57 ++ // + span: no-location + // + literal: Const { ty: E, val: Value(Scalar(0x00000004)) } + _3 = [const Scalar(0x00000004): E]; // scope 1 at $DIR/invalid_constant.rs:+13:24: +13:60 + // mir::Constant -+ // + span: $DIR/invalid_constant.rs:28:24: 28:60 ++ // + span: no-location + // + literal: Const { ty: E, val: Value(Scalar(0x00000004)) } StorageDead(_4); // scope 1 at $DIR/invalid_constant.rs:+13:59: +13:60 StorageDead(_5); // scope 1 at $DIR/invalid_constant.rs:+13:60: +13:61 diff --git a/tests/mir-opt/funky_arms.float_to_exponential_common.ConstProp.diff b/tests/mir-opt/funky_arms.float_to_exponential_common.ConstProp.diff index 1f5c533815d7..ec063294856c 100644 --- a/tests/mir-opt/funky_arms.float_to_exponential_common.ConstProp.diff +++ b/tests/mir-opt/funky_arms.float_to_exponential_common.ConstProp.diff @@ -54,7 +54,7 @@ - _6 = MinusPlus; // scope 1 at $DIR/funky_arms.rs:+10:17: +10:41 + _6 = const MinusPlus; // scope 1 at $DIR/funky_arms.rs:+10:17: +10:41 + // mir::Constant -+ // + span: $DIR/funky_arms.rs:21:17: 21:41 ++ // + span: no-location + // + literal: Const { ty: Sign, val: Value(Scalar(0x01)) } goto -> bb4; // scope 1 at $DIR/funky_arms.rs:+10:17: +10:41 } @@ -63,7 +63,7 @@ - _6 = Minus; // scope 1 at $DIR/funky_arms.rs:+9:18: +9:38 + _6 = const Minus; // scope 1 at $DIR/funky_arms.rs:+9:18: +9:38 + // mir::Constant -+ // + span: $DIR/funky_arms.rs:20:18: 20:38 ++ // + span: no-location + // + literal: Const { ty: Sign, val: Value(Scalar(0x00)) } goto -> bb4; // scope 1 at $DIR/funky_arms.rs:+9:18: +9:38 } From 081bc75743c1d6f4a3f41df4444f70f82fbb79ab Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 08:20:10 +0000 Subject: [PATCH 04/13] Assume the frame has all the locals. --- .../src/interpret/eval_context.rs | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs index 39c741912582..0918ffcd9821 100644 --- a/compiler/rustc_const_eval/src/interpret/eval_context.rs +++ b/compiler/rustc_const_eval/src/interpret/eval_context.rs @@ -536,24 +536,20 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { local: mir::Local, layout: Option>, ) -> InterpResult<'tcx, TyAndLayout<'tcx>> { - // `const_prop` runs into this with an invalid (empty) frame, so we - // have to support that case (mostly by skipping all caching). - match frame.locals.get(local).and_then(|state| state.layout.get()) { - None => { - let layout = from_known_layout(self.tcx, self.param_env, layout, || { - let local_ty = frame.body.local_decls[local].ty; - let local_ty = - self.subst_from_frame_and_normalize_erasing_regions(frame, local_ty)?; - self.layout_of(local_ty) - })?; - if let Some(state) = frame.locals.get(local) { - // Layouts of locals are requested a lot, so we cache them. - state.layout.set(Some(layout)); - } - Ok(layout) - } - Some(layout) => Ok(layout), + let state = &frame.locals[local]; + if let Some(layout) = state.layout.get() { + return Ok(layout); } + + let layout = from_known_layout(self.tcx, self.param_env, layout, || { + let local_ty = frame.body.local_decls[local].ty; + let local_ty = self.subst_from_frame_and_normalize_erasing_regions(frame, local_ty)?; + self.layout_of(local_ty) + })?; + + // Layouts of locals are requested a lot, so we cache them. + state.layout.set(Some(layout)); + Ok(layout) } /// Returns the actual dynamic size and alignment of the place at the given type. From 24dbf9c1123716ab0589772b301468d34a7a8a9b Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 14:32:16 +0000 Subject: [PATCH 05/13] Only assign value in remove_const. --- compiler/rustc_mir_transform/src/const_prop.rs | 12 ++++-------- compiler/rustc_mir_transform/src/const_prop_lint.rs | 10 +++------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index afc5f08b2d2b..1cc4e21ea51d 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -1,8 +1,6 @@ //! Propagates constants for early reporting of statically known //! assertion failures -use std::cell::Cell; - use either::Right; use rustc_const_eval::const_eval::CheckAlignment; @@ -25,8 +23,8 @@ use rustc_trait_selection::traits; use crate::MirPass; use rustc_const_eval::interpret::{ self, compile_time_machine, AllocId, ConstAllocation, ConstValue, CtfeValidationMode, Frame, - ImmTy, Immediate, InterpCx, InterpResult, LocalState, LocalValue, MemoryKind, OpTy, PlaceTy, - Pointer, Scalar, StackPopCleanup, StackPopUnwind, + ImmTy, Immediate, InterpCx, InterpResult, LocalValue, MemoryKind, OpTy, PlaceTy, Pointer, + Scalar, StackPopCleanup, StackPopUnwind, }; /// The maximum number of bytes that we'll allocate space for a local or the return value. @@ -437,10 +435,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { /// Remove `local` from the pool of `Locals`. Allows writing to them, /// but not reading from them anymore. fn remove_const(ecx: &mut InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, local: Local) { - ecx.frame_mut().locals[local] = LocalState { - value: LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit)), - layout: Cell::new(None), - }; + ecx.frame_mut().locals[local].value = + LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit)); } /// Returns the value, if any, of evaluating `c`. diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 030d79ac22de..f89454170067 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -1,13 +1,11 @@ //! Propagates constants for early reporting of statically known //! assertion failures -use std::cell::Cell; - use either::{Left, Right}; use rustc_const_eval::interpret::Immediate; use rustc_const_eval::interpret::{ - self, InterpCx, InterpResult, LocalState, LocalValue, MemoryKind, OpTy, Scalar, StackPopCleanup, + self, InterpCx, InterpResult, LocalValue, MemoryKind, OpTy, Scalar, StackPopCleanup, }; use rustc_hir::def::DefKind; use rustc_hir::HirId; @@ -254,10 +252,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { /// Remove `local` from the pool of `Locals`. Allows writing to them, /// but not reading from them anymore. fn remove_const(ecx: &mut InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>, local: Local) { - ecx.frame_mut().locals[local] = LocalState { - value: LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit)), - layout: Cell::new(None), - }; + ecx.frame_mut().locals[local].value = + LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit)); } fn lint_root(&self, source_info: SourceInfo) -> Option { From 9928d0e566bebe5ffca533ef332a18375a70b83f Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 15:04:53 +0000 Subject: [PATCH 06/13] Remove OnlyPropagateInto. --- .../rustc_mir_transform/src/const_prop.rs | 35 ++++--------------- .../src/const_prop_lint.rs | 4 +-- .../bad_op_mod_by_zero.main.ConstProp.diff | 8 +++-- 3 files changed, 13 insertions(+), 34 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 1cc4e21ea51d..a78b36c65f31 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -718,8 +718,6 @@ pub enum ConstPropMode { FullConstProp, /// The `Local` can only be propagated into and from its own block. OnlyInsideOwnBlock, - /// The `Local` can be propagated into but reads cannot be propagated. - OnlyPropagateInto, /// The `Local` cannot be part of propagation at all. Any statement /// referencing it either for reading or writing will not get propagated. NoPropagation, @@ -729,8 +727,6 @@ pub struct CanConstProp { can_const_prop: IndexVec, // False at the beginning. Once set, no more assignments are allowed to that local. found_assignment: BitSet, - // Cache of locals' information - local_kinds: IndexVec, } impl CanConstProp { @@ -743,10 +739,6 @@ impl CanConstProp { let mut cpv = CanConstProp { can_const_prop: IndexVec::from_elem(ConstPropMode::FullConstProp, &body.local_decls), found_assignment: BitSet::new_empty(body.local_decls.len()), - local_kinds: IndexVec::from_fn_n( - |local| body.local_kind(local), - body.local_decls.len(), - ), }; for (local, val) in cpv.can_const_prop.iter_enumerated_mut() { let ty = body.local_decls[local].ty; @@ -759,24 +751,10 @@ impl CanConstProp { continue; } } - // Cannot use args at all - // Cannot use locals because if x < y { y - x } else { x - y } would - // lint for x != y - // FIXME(oli-obk): lint variables until they are used in a condition - // FIXME(oli-obk): lint if return value is constant - if cpv.local_kinds[local] == LocalKind::Arg { - *val = ConstPropMode::OnlyPropagateInto; - trace!( - "local {:?} can't be const propagated because it's a function argument", - local - ); - } else if cpv.local_kinds[local] == LocalKind::Var { - *val = ConstPropMode::OnlyInsideOwnBlock; - trace!( - "local {:?} will only be propagated inside its block, because it's a user variable", - local - ); - } + } + // Consider that arguments are assigned on entry. + for arg in body.args_iter() { + cpv.found_assignment.insert(arg); } cpv.visit_body(&body); cpv.can_const_prop @@ -806,7 +784,6 @@ impl Visitor<'_> for CanConstProp { // states as applicable. ConstPropMode::OnlyInsideOwnBlock => {} ConstPropMode::NoPropagation => {} - ConstPropMode::OnlyPropagateInto => {} other @ ConstPropMode::FullConstProp => { trace!( "local {:?} can't be propagated because of multiple assignments. Previous state: {:?}", @@ -897,7 +874,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { place.local ); } - ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { + ConstPropMode::NoPropagation => { trace!("can't propagate into {:?}", place); if place.local != RETURN_PLACE { Self::remove_const(&mut self.ecx, place.local); @@ -933,7 +910,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { Self::remove_const(&mut self.ecx, place.local); } } - ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { + ConstPropMode::NoPropagation => { Self::remove_const(&mut self.ecx, place.local); } } diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index f89454170067..c0a1e502d852 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -530,7 +530,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { place.local ); } - ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { + ConstPropMode::NoPropagation => { trace!("can't propagate into {:?}", place); if place.local != RETURN_PLACE { Self::remove_const(&mut self.ecx, place.local); @@ -567,7 +567,7 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { Self::remove_const(&mut self.ecx, place.local); } } - ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => { + ConstPropMode::NoPropagation => { Self::remove_const(&mut self.ecx, place.local); } } diff --git a/tests/mir-opt/const_prop/bad_op_mod_by_zero.main.ConstProp.diff b/tests/mir-opt/const_prop/bad_op_mod_by_zero.main.ConstProp.diff index ae9ffd519a14..bedfa5992ad5 100644 --- a/tests/mir-opt/const_prop/bad_op_mod_by_zero.main.ConstProp.diff +++ b/tests/mir-opt/const_prop/bad_op_mod_by_zero.main.ConstProp.diff @@ -27,17 +27,19 @@ } bb1: { - _5 = Eq(_1, const -1_i32); // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 +- _5 = Eq(_1, const -1_i32); // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 - _6 = Eq(const 1_i32, const i32::MIN); // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 - _7 = BitAnd(move _5, move _6); // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 - assert(!move _7, "attempt to compute the remainder of `{} % {}`, which would overflow", const 1_i32, _1) -> bb2; // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 ++ _5 = const false; // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 + _6 = const false; // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 + _7 = const false; // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 -+ assert(!const false, "attempt to compute the remainder of `{} % {}`, which would overflow", const 1_i32, _1) -> bb2; // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 ++ assert(!const false, "attempt to compute the remainder of `{} % {}`, which would overflow", const 1_i32, const 0_i32) -> bb2; // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 } bb2: { - _2 = Rem(const 1_i32, _1); // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 +- _2 = Rem(const 1_i32, _1); // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 ++ _2 = Rem(const 1_i32, const 0_i32); // scope 1 at $DIR/bad_op_mod_by_zero.rs:+2:14: +2:19 StorageDead(_2); // scope 1 at $DIR/bad_op_mod_by_zero.rs:+3:1: +3:2 return; // scope 0 at $DIR/bad_op_mod_by_zero.rs:+3:2: +3:2 } From d97a7ce69b35b42778709333c46d87c08c2848e7 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 15:20:57 +0000 Subject: [PATCH 07/13] Refactor tracking of writes. --- .../rustc_mir_transform/src/const_prop.rs | 82 ++++++++----------- .../src/const_prop_lint.rs | 48 +++++------ 2 files changed, 58 insertions(+), 72 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index a78b36c65f31..cf5eb697b24b 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -4,7 +4,6 @@ use either::Right; use rustc_const_eval::const_eval::CheckAlignment; -use rustc_data_structures::fx::FxHashSet; use rustc_hir::def::DefKind; use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; @@ -152,24 +151,12 @@ impl<'tcx> MirPass<'tcx> for ConstProp { pub struct ConstPropMachine<'mir, 'tcx> { /// The virtual call stack. stack: Vec>, - /// `OnlyInsideOwnBlock` locals that were written in the current block get erased at the end. - pub written_only_inside_own_block_locals: FxHashSet, - /// Locals that need to be cleared after every block terminates. - pub only_propagate_inside_block_locals: BitSet, pub can_const_prop: IndexVec, } impl ConstPropMachine<'_, '_> { - pub fn new( - only_propagate_inside_block_locals: BitSet, - can_const_prop: IndexVec, - ) -> Self { - Self { - stack: Vec::new(), - written_only_inside_own_block_locals: Default::default(), - only_propagate_inside_block_locals, - can_const_prop, - } + pub fn new(can_const_prop: IndexVec) -> Self { + Self { stack: Vec::new(), can_const_prop } } } @@ -255,16 +242,14 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx> frame: usize, local: Local, ) -> InterpResult<'tcx, &'a mut interpret::Operand> { - if ecx.machine.can_const_prop[local] == ConstPropMode::NoPropagation { - throw_machine_stop_str!("tried to write to a local that is marked as not propagatable") - } - if frame == 0 && ecx.machine.only_propagate_inside_block_locals.contains(local) { - trace!( - "mutating local {:?} which is restricted to its block. \ - Will remove it from const-prop after block is finished.", - local - ); - ecx.machine.written_only_inside_own_block_locals.insert(local); + assert_eq!(frame, 0); + match ecx.machine.can_const_prop[local] { + ConstPropMode::NoPropagation => { + throw_machine_stop_str!( + "tried to write to a local that is marked as not propagatable" + ) + } + ConstPropMode::OnlyInsideOwnBlock | ConstPropMode::FullConstProp => {} } ecx.machine.stack[frame].locals[local].access_mut() } @@ -369,17 +354,11 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let param_env = tcx.param_env_reveal_all_normalized(def_id); let can_const_prop = CanConstProp::check(tcx, param_env, body); - let mut only_propagate_inside_block_locals = BitSet::new_empty(can_const_prop.len()); - for (l, mode) in can_const_prop.iter_enumerated() { - if *mode == ConstPropMode::OnlyInsideOwnBlock { - only_propagate_inside_block_locals.insert(l); - } - } let mut ecx = InterpCx::new( tcx, tcx.def_span(def_id), param_env, - ConstPropMachine::new(only_propagate_inside_block_locals, can_const_prop), + ConstPropMachine::new(can_const_prop), ); let ret_layout = ecx @@ -977,26 +956,33 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_basic_block_data(&mut self, block: BasicBlock, data: &mut BasicBlockData<'tcx>) { self.super_basic_block_data(block, data); + let ensure_not_propagated = |this: &mut Self, local: Local| { + if cfg!(debug_assertions) { + assert!( + this.get_const(local.into()).is_none() + || this + .layout_of(this.local_decls[local].ty) + .map_or(true, |layout| layout.is_zst()), + "failed to remove values for `{local:?}`, value={:?}", + this.get_const(local.into()), + ) + } + }; + // We remove all Locals which are restricted in propagation to their containing blocks and // which were modified in the current block. // Take it out of the ecx so we can get a mutable reference to the ecx for `remove_const`. - let mut locals = std::mem::take(&mut self.ecx.machine.written_only_inside_own_block_locals); - for &local in locals.iter() { - Self::remove_const(&mut self.ecx, local); - } - locals.clear(); - // Put it back so we reuse the heap of the storage - self.ecx.machine.written_only_inside_own_block_locals = locals; - if cfg!(debug_assertions) { - // Ensure we are correctly erasing locals with the non-debug-assert logic. - for local in self.ecx.machine.only_propagate_inside_block_locals.iter() { - assert!( - self.get_const(local.into()).is_none() - || self - .layout_of(self.local_decls[local].ty) - .map_or(true, |layout| layout.is_zst()) - ) + let can_const_prop = std::mem::take(&mut self.ecx.machine.can_const_prop); + for (local, &mode) in can_const_prop.iter_enumerated() { + match mode { + ConstPropMode::FullConstProp => {} + ConstPropMode::NoPropagation => ensure_not_propagated(self, local), + ConstPropMode::OnlyInsideOwnBlock => { + Self::remove_const(&mut self.ecx, local); + ensure_not_propagated(self, local); + } } } + self.ecx.machine.can_const_prop = can_const_prop; } } diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index c0a1e502d852..52c86fae7b49 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -9,7 +9,6 @@ use rustc_const_eval::interpret::{ }; use rustc_hir::def::DefKind; use rustc_hir::HirId; -use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::*; @@ -179,17 +178,11 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let param_env = tcx.param_env_reveal_all_normalized(def_id); let can_const_prop = CanConstProp::check(tcx, param_env, body); - let mut only_propagate_inside_block_locals = BitSet::new_empty(can_const_prop.len()); - for (l, mode) in can_const_prop.iter_enumerated() { - if *mode == ConstPropMode::OnlyInsideOwnBlock { - only_propagate_inside_block_locals.insert(l); - } - } let mut ecx = InterpCx::new( tcx, tcx.def_span(def_id), param_env, - ConstPropMachine::new(only_propagate_inside_block_locals, can_const_prop), + ConstPropMachine::new(can_const_prop), ); let ret_layout = ecx @@ -687,26 +680,33 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_basic_block_data(&mut self, block: BasicBlock, data: &BasicBlockData<'tcx>) { self.super_basic_block_data(block, data); + let ensure_not_propagated = |this: &mut Self, local: Local| { + if cfg!(debug_assertions) { + assert!( + this.get_const(local.into()).is_none() + || this + .layout_of(this.local_decls[local].ty) + .map_or(true, |layout| layout.is_zst()), + "failed to remove values for `{local:?}`, value={:?}", + this.get_const(local.into()), + ) + } + }; + // We remove all Locals which are restricted in propagation to their containing blocks and // which were modified in the current block. // Take it out of the ecx so we can get a mutable reference to the ecx for `remove_const`. - let mut locals = std::mem::take(&mut self.ecx.machine.written_only_inside_own_block_locals); - for &local in locals.iter() { - Self::remove_const(&mut self.ecx, local); - } - locals.clear(); - // Put it back so we reuse the heap of the storage - self.ecx.machine.written_only_inside_own_block_locals = locals; - if cfg!(debug_assertions) { - // Ensure we are correctly erasing locals with the non-debug-assert logic. - for local in self.ecx.machine.only_propagate_inside_block_locals.iter() { - assert!( - self.get_const(local.into()).is_none() - || self - .layout_of(self.local_decls[local].ty) - .map_or(true, |layout| layout.is_zst()) - ) + let can_const_prop = std::mem::take(&mut self.ecx.machine.can_const_prop); + for (local, &mode) in can_const_prop.iter_enumerated() { + match mode { + ConstPropMode::FullConstProp => {} + ConstPropMode::NoPropagation => ensure_not_propagated(self, local), + ConstPropMode::OnlyInsideOwnBlock => { + Self::remove_const(&mut self.ecx, local); + ensure_not_propagated(self, local); + } } } + self.ecx.machine.can_const_prop = can_const_prop; } } From f00be8b77bbbc614662a1e8349463aca45c01e62 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 15:40:38 +0000 Subject: [PATCH 08/13] Recurse into statement before applying its effect. --- compiler/rustc_mir_transform/src/const_prop.rs | 6 ++++-- compiler/rustc_mir_transform/src/const_prop_lint.rs | 6 ++++-- tests/ui/consts/const-err-late.stderr | 12 ++++++------ 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index cf5eb697b24b..fa8c22ceb40b 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -829,6 +829,10 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Location) { trace!("visit_statement: {:?}", statement); + + // Recurse into statement before applying the assignment. + self.super_statement(statement, location); + match statement.kind { StatementKind::Assign(box (place, ref mut rval)) => { let can_const_prop = self.ecx.machine.can_const_prop[place.local]; @@ -905,8 +909,6 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { } _ => {} } - - self.super_statement(statement, location); } fn visit_terminator(&mut self, terminator: &mut Terminator<'tcx>, location: Location) { diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 52c86fae7b49..b7835f842ba9 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -511,6 +511,10 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { trace!("visit_statement: {:?}", statement); let source_info = statement.source_info; self.source_info = Some(source_info); + + // Recurse into statement before applying the assignment. + self.super_statement(statement, location); + match statement.kind { StatementKind::Assign(box (place, ref rval)) => { let can_const_prop = self.ecx.machine.can_const_prop[place.local]; @@ -576,8 +580,6 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { } _ => {} } - - self.super_statement(statement, location); } fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) { diff --git a/tests/ui/consts/const-err-late.stderr b/tests/ui/consts/const-err-late.stderr index cb0cab2444bf..192b9ba204be 100644 --- a/tests/ui/consts/const-err-late.stderr +++ b/tests/ui/consts/const-err-late.stderr @@ -10,12 +10,6 @@ note: erroneous constant used LL | black_box((S::::FOO, S::::FOO)); | ^^^^^^^^^^^^^ -note: erroneous constant used - --> $DIR/const-err-late.rs:19:16 - | -LL | black_box((S::::FOO, S::::FOO)); - | ^^^^^^^^^^^^^ - error[E0080]: evaluation of `S::::FOO` failed --> $DIR/const-err-late.rs:13:21 | @@ -34,6 +28,12 @@ note: erroneous constant used LL | black_box((S::::FOO, S::::FOO)); | ^^^^^^^^^^^^^ +note: erroneous constant used + --> $DIR/const-err-late.rs:19:16 + | +LL | black_box((S::::FOO, S::::FOO)); + | ^^^^^^^^^^^^^ + error: aborting due to 2 previous errors For more information about this error, try `rustc --explain E0080`. From b55c4f8312d48c83fc4dc3ef8b9f167c97a4d459 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 16:34:11 +0000 Subject: [PATCH 09/13] Separate checking rvalue from evaluation. --- .../rustc_mir_transform/src/const_prop.rs | 116 +++++++++--------- .../src/const_prop_lint.rs | 101 +++++++-------- 2 files changed, 104 insertions(+), 113 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index fa8c22ceb40b..6e590981357e 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -470,7 +470,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } } - fn const_prop(&mut self, rvalue: &Rvalue<'tcx>, place: Place<'tcx>) -> Option<()> { + fn check_rvalue(&mut self, rvalue: &Rvalue<'tcx>) -> Option<()> { // Perform any special handling for specific Rvalue types. // Generally, checks here fall into one of two categories: // 1. Additional checking to provide useful lints to the user @@ -527,7 +527,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } - self.eval_rvalue_with_identities(rvalue, place) + Some(()) } // Attempt to use algebraic identities to eliminate constant expressions @@ -595,7 +595,16 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { })) } - fn replace_with_const(&mut self, rval: &mut Rvalue<'tcx>, value: &OpTy<'tcx>) { + fn replace_with_const(&mut self, place: Place<'tcx>, rval: &mut Rvalue<'tcx>) { + // 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). + let Some(ref value) = self.get_const(place) else { return }; + if !self.should_const_prop(value) { + return; + } + trace!("replacing {:?}={:?} with {:?}", place, rval, value); + if let Rvalue::Use(Operand::Constant(c)) = rval { match c.literal { ConstantKind::Ty(c) if matches!(c.kind(), ConstKind::Unevaluated(..)) => {} @@ -688,6 +697,19 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { _ => false, } } + + fn ensure_not_propagated(&mut self, local: Local) { + if cfg!(debug_assertions) { + assert!( + self.get_const(local.into()).is_none() + || self + .layout_of(self.local_decls[local].ty) + .map_or(true, |layout| layout.is_zst()), + "failed to remove values for `{local:?}`, value={:?}", + self.get_const(local.into()), + ) + } + } } /// The mode that `ConstProp` is allowed to run in for a given `Local`. @@ -827,44 +849,23 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { self.eval_constant(constant); } - fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Location) { - trace!("visit_statement: {:?}", statement); - - // Recurse into statement before applying the assignment. - self.super_statement(statement, location); - - 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); - 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. \ - Will remove it from const-prop after block is finished. Local: {:?}", - place.local - ); - } - ConstPropMode::NoPropagation => { - trace!("can't propagate into {:?}", place); - if place.local != RETURN_PLACE { - Self::remove_const(&mut self.ecx, place.local); - } - } - ConstPropMode::FullConstProp => {} - } + fn visit_assign( + &mut self, + place: &mut Place<'tcx>, + rvalue: &mut Rvalue<'tcx>, + location: Location, + ) { + self.super_assign(place, rvalue, location); + + let Some(()) = self.check_rvalue(rvalue) else { return }; + + match self.ecx.machine.can_const_prop[place.local] { + // Do nothing if the place is indirect. + _ if place.is_indirect() => {} + ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), + ConstPropMode::OnlyInsideOwnBlock | ConstPropMode::FullConstProp => { + if let Some(()) = self.eval_rvalue_with_identities(rvalue, *place) { + self.replace_with_const(*place, rvalue); } else { // Const prop failed, so erase the destination, ensuring that whatever happens // from here on, does not know about the previous value. @@ -884,8 +885,21 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { Self::remove_const(&mut self.ecx, place.local); } } + } + } + + fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Location) { + trace!("visit_statement: {:?}", statement); + + // Recurse into statement before applying the assignment. + self.super_statement(statement, location); + + match statement.kind { StatementKind::SetDiscriminant { ref place, .. } => { match self.ecx.machine.can_const_prop[place.local] { + // Do nothing if the place is indirect. + _ if place.is_indirect() => {} + ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => { if self.ecx.statement(statement).is_ok() { trace!("propped discriminant into {:?}", place); @@ -893,9 +907,6 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { Self::remove_const(&mut self.ecx, place.local); } } - ConstPropMode::NoPropagation => { - Self::remove_const(&mut self.ecx, place.local); - } } } StatementKind::StorageLive(local) => { @@ -958,19 +969,6 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_basic_block_data(&mut self, block: BasicBlock, data: &mut BasicBlockData<'tcx>) { self.super_basic_block_data(block, data); - let ensure_not_propagated = |this: &mut Self, local: Local| { - if cfg!(debug_assertions) { - assert!( - this.get_const(local.into()).is_none() - || this - .layout_of(this.local_decls[local].ty) - .map_or(true, |layout| layout.is_zst()), - "failed to remove values for `{local:?}`, value={:?}", - this.get_const(local.into()), - ) - } - }; - // We remove all Locals which are restricted in propagation to their containing blocks and // which were modified in the current block. // Take it out of the ecx so we can get a mutable reference to the ecx for `remove_const`. @@ -978,10 +976,10 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { for (local, &mode) in can_const_prop.iter_enumerated() { match mode { ConstPropMode::FullConstProp => {} - ConstPropMode::NoPropagation => ensure_not_propagated(self, local), + ConstPropMode::NoPropagation => self.ensure_not_propagated(local), ConstPropMode::OnlyInsideOwnBlock => { Self::remove_const(&mut self.ecx, local); - ensure_not_propagated(self, local); + self.ensure_not_propagated(local); } } } diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index b7835f842ba9..28198781fd97 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -405,12 +405,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { Some(()) } - fn const_prop( - &mut self, - rvalue: &Rvalue<'tcx>, - source_info: SourceInfo, - place: Place<'tcx>, - ) -> Option<()> { + fn check_rvalue(&mut self, rvalue: &Rvalue<'tcx>, source_info: SourceInfo) -> Option<()> { // Perform any special handling for specific Rvalue types. // Generally, checks here fall into one of two categories: // 1. Additional checking to provide useful lints to the user @@ -486,7 +481,20 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } - self.use_ecx(source_info, |this| this.ecx.eval_rvalue_into_place(rvalue, place)) + Some(()) + } + + fn ensure_not_propagated(&mut self, local: Local) { + if cfg!(debug_assertions) { + assert!( + self.get_const(local.into()).is_none() + || self + .layout_of(self.local_decls[local].ty) + .map_or(true, |layout| layout.is_zst()), + "failed to remove values for `{local:?}`, value={:?}", + self.get_const(local.into()), + ) + } } } @@ -507,35 +515,21 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { self.eval_constant(constant, self.source_info.unwrap()); } - fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { - trace!("visit_statement: {:?}", statement); - let source_info = statement.source_info; - self.source_info = Some(source_info); - - // Recurse into statement before applying the assignment. - self.super_statement(statement, location); - - match statement.kind { - StatementKind::Assign(box (place, ref rval)) => { - let can_const_prop = self.ecx.machine.can_const_prop[place.local]; - if let Some(()) = self.const_prop(rval, source_info, place) { - 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::NoPropagation => { - trace!("can't propagate into {:?}", place); - if place.local != RETURN_PLACE { - Self::remove_const(&mut self.ecx, place.local); - } - } - ConstPropMode::FullConstProp => {} - } - } else { + fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { + self.super_assign(place, rvalue, location); + + let source_info = self.source_info.unwrap(); + let Some(()) = self.check_rvalue(rvalue, source_info) else { return }; + + match self.ecx.machine.can_const_prop[place.local] { + // Do nothing if the place is indirect. + _ if place.is_indirect() => {} + ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), + ConstPropMode::OnlyInsideOwnBlock | ConstPropMode::FullConstProp => { + if self + .use_ecx(source_info, |this| this.ecx.eval_rvalue_into_place(rvalue, *place)) + .is_none() + { // 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 @@ -554,8 +548,23 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { Self::remove_const(&mut self.ecx, place.local); } } + } + } + + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { + trace!("visit_statement: {:?}", statement); + let source_info = statement.source_info; + self.source_info = Some(source_info); + + // Recurse into statement before applying the assignment. + self.super_statement(statement, location); + + match statement.kind { StatementKind::SetDiscriminant { ref place, .. } => { match self.ecx.machine.can_const_prop[place.local] { + // Do nothing if the place is indirect. + _ if place.is_indirect() => {} + ConstPropMode::NoPropagation => self.ensure_not_propagated(place.local), ConstPropMode::FullConstProp | ConstPropMode::OnlyInsideOwnBlock => { if self.use_ecx(source_info, |this| this.ecx.statement(statement)).is_some() { @@ -564,9 +573,6 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { Self::remove_const(&mut self.ecx, place.local); } } - ConstPropMode::NoPropagation => { - Self::remove_const(&mut self.ecx, place.local); - } } } StatementKind::StorageLive(local) => { @@ -682,19 +688,6 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_basic_block_data(&mut self, block: BasicBlock, data: &BasicBlockData<'tcx>) { self.super_basic_block_data(block, data); - let ensure_not_propagated = |this: &mut Self, local: Local| { - if cfg!(debug_assertions) { - assert!( - this.get_const(local.into()).is_none() - || this - .layout_of(this.local_decls[local].ty) - .map_or(true, |layout| layout.is_zst()), - "failed to remove values for `{local:?}`, value={:?}", - this.get_const(local.into()), - ) - } - }; - // We remove all Locals which are restricted in propagation to their containing blocks and // which were modified in the current block. // Take it out of the ecx so we can get a mutable reference to the ecx for `remove_const`. @@ -702,10 +695,10 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { for (local, &mode) in can_const_prop.iter_enumerated() { match mode { ConstPropMode::FullConstProp => {} - ConstPropMode::NoPropagation => ensure_not_propagated(self, local), + ConstPropMode::NoPropagation => self.ensure_not_propagated(local), ConstPropMode::OnlyInsideOwnBlock => { Self::remove_const(&mut self.ecx, local); - ensure_not_propagated(self, local); + self.ensure_not_propagated(local); } } } From 0d56034a25b149ac51ae2b497c3f33ca8c838786 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 17:12:42 +0000 Subject: [PATCH 10/13] Make comment more explicit. --- compiler/rustc_mir_transform/src/const_prop.rs | 3 ++- compiler/rustc_mir_transform/src/const_prop_lint.rs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 6e590981357e..334dd90d4ae4 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -891,7 +891,8 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Location) { trace!("visit_statement: {:?}", statement); - // Recurse into statement before applying the assignment. + // We want to evaluate operands before any change to the assigned-to value, + // so we recurse first. self.super_statement(statement, location); match statement.kind { diff --git a/compiler/rustc_mir_transform/src/const_prop_lint.rs b/compiler/rustc_mir_transform/src/const_prop_lint.rs index 28198781fd97..cbf29380ff24 100644 --- a/compiler/rustc_mir_transform/src/const_prop_lint.rs +++ b/compiler/rustc_mir_transform/src/const_prop_lint.rs @@ -556,7 +556,8 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> { let source_info = statement.source_info; self.source_info = Some(source_info); - // Recurse into statement before applying the assignment. + // We want to evaluate operands before any change to the assigned-to value, + // so we recurse first. self.super_statement(statement, location); match statement.kind { From a5a01b21e62892c3f8fe735a3789d10fb31d9916 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Tue, 7 Mar 2023 17:51:48 +0000 Subject: [PATCH 11/13] Fortify clippy tests. --- src/tools/clippy/tests/ui/erasing_op.rs | 8 +++++--- src/tools/clippy/tests/ui/erasing_op.stderr | 10 +++++----- src/tools/clippy/tests/ui/integer_arithmetic.rs | 2 +- .../tests/ui/overflow_check_conditional.rs | 9 +++++---- .../tests/ui/overflow_check_conditional.stderr | 16 ++++++++-------- 5 files changed, 24 insertions(+), 21 deletions(-) diff --git a/src/tools/clippy/tests/ui/erasing_op.rs b/src/tools/clippy/tests/ui/erasing_op.rs index ae2fad0086da..74985029e008 100644 --- a/src/tools/clippy/tests/ui/erasing_op.rs +++ b/src/tools/clippy/tests/ui/erasing_op.rs @@ -31,9 +31,7 @@ impl core::ops::Mul for Vec1 { #[allow(clippy::no_effect)] #[warn(clippy::erasing_op)] -fn main() { - let x: u8 = 0; - +fn test(x: u8) { x * 0; 0 & x; 0 / x; @@ -41,3 +39,7 @@ fn main() { 0 * Vec1 { x: 5 }; Vec1 { x: 5 } * 0; } + +fn main() { + test(0) +} diff --git a/src/tools/clippy/tests/ui/erasing_op.stderr b/src/tools/clippy/tests/ui/erasing_op.stderr index 165ed9bfe58b..97941252355a 100644 --- a/src/tools/clippy/tests/ui/erasing_op.stderr +++ b/src/tools/clippy/tests/ui/erasing_op.stderr @@ -1,5 +1,5 @@ error: this operation will always return zero. This is likely not the intended outcome - --> $DIR/erasing_op.rs:37:5 + --> $DIR/erasing_op.rs:35:5 | LL | x * 0; | ^^^^^ @@ -7,25 +7,25 @@ LL | x * 0; = note: `-D clippy::erasing-op` implied by `-D warnings` error: this operation will always return zero. This is likely not the intended outcome - --> $DIR/erasing_op.rs:38:5 + --> $DIR/erasing_op.rs:36:5 | LL | 0 & x; | ^^^^^ error: this operation will always return zero. This is likely not the intended outcome - --> $DIR/erasing_op.rs:39:5 + --> $DIR/erasing_op.rs:37:5 | LL | 0 / x; | ^^^^^ error: this operation will always return zero. This is likely not the intended outcome - --> $DIR/erasing_op.rs:41:5 + --> $DIR/erasing_op.rs:39:5 | LL | 0 * Vec1 { x: 5 }; | ^^^^^^^^^^^^^^^^^ error: this operation will always return zero. This is likely not the intended outcome - --> $DIR/erasing_op.rs:42:5 + --> $DIR/erasing_op.rs:40:5 | LL | Vec1 { x: 5 } * 0; | ^^^^^^^^^^^^^^^^^ diff --git a/src/tools/clippy/tests/ui/integer_arithmetic.rs b/src/tools/clippy/tests/ui/integer_arithmetic.rs index 67f24b4548aa..8dfdee662b9d 100644 --- a/src/tools/clippy/tests/ui/integer_arithmetic.rs +++ b/src/tools/clippy/tests/ui/integer_arithmetic.rs @@ -4,7 +4,7 @@ #[rustfmt::skip] fn main() { let mut i = 1i32; - let mut var1 = 0i32; + let mut var1 = 13i32; let mut var2 = -1i32; 1 + i; i * 2; diff --git a/src/tools/clippy/tests/ui/overflow_check_conditional.rs b/src/tools/clippy/tests/ui/overflow_check_conditional.rs index 5db75f5291be..e1e30114081e 100644 --- a/src/tools/clippy/tests/ui/overflow_check_conditional.rs +++ b/src/tools/clippy/tests/ui/overflow_check_conditional.rs @@ -1,9 +1,6 @@ #![warn(clippy::overflow_check_conditional)] -fn main() { - let a: u32 = 1; - let b: u32 = 2; - let c: u32 = 3; +fn test(a: u32, b: u32, c: u32) { if a + b < a {} if a > a + b {} if a + b < b {} @@ -23,3 +20,7 @@ fn main() { if i > i + j {} if i - j < i {} } + +fn main() { + test(1, 2, 3) +} diff --git a/src/tools/clippy/tests/ui/overflow_check_conditional.stderr b/src/tools/clippy/tests/ui/overflow_check_conditional.stderr index 1b8b146b60ae..92d1d8ef911e 100644 --- a/src/tools/clippy/tests/ui/overflow_check_conditional.stderr +++ b/src/tools/clippy/tests/ui/overflow_check_conditional.stderr @@ -1,5 +1,5 @@ error: you are trying to use classic C overflow conditions that will fail in Rust - --> $DIR/overflow_check_conditional.rs:7:8 + --> $DIR/overflow_check_conditional.rs:4:8 | LL | if a + b < a {} | ^^^^^^^^^ @@ -7,43 +7,43 @@ LL | if a + b < a {} = note: `-D clippy::overflow-check-conditional` implied by `-D warnings` error: you are trying to use classic C overflow conditions that will fail in Rust - --> $DIR/overflow_check_conditional.rs:8:8 + --> $DIR/overflow_check_conditional.rs:5:8 | LL | if a > a + b {} | ^^^^^^^^^ error: you are trying to use classic C overflow conditions that will fail in Rust - --> $DIR/overflow_check_conditional.rs:9:8 + --> $DIR/overflow_check_conditional.rs:6:8 | LL | if a + b < b {} | ^^^^^^^^^ error: you are trying to use classic C overflow conditions that will fail in Rust - --> $DIR/overflow_check_conditional.rs:10:8 + --> $DIR/overflow_check_conditional.rs:7:8 | LL | if b > a + b {} | ^^^^^^^^^ error: you are trying to use classic C underflow conditions that will fail in Rust - --> $DIR/overflow_check_conditional.rs:11:8 + --> $DIR/overflow_check_conditional.rs:8:8 | LL | if a - b > b {} | ^^^^^^^^^ error: you are trying to use classic C underflow conditions that will fail in Rust - --> $DIR/overflow_check_conditional.rs:12:8 + --> $DIR/overflow_check_conditional.rs:9:8 | LL | if b < a - b {} | ^^^^^^^^^ error: you are trying to use classic C underflow conditions that will fail in Rust - --> $DIR/overflow_check_conditional.rs:13:8 + --> $DIR/overflow_check_conditional.rs:10:8 | LL | if a - b > a {} | ^^^^^^^^^ error: you are trying to use classic C underflow conditions that will fail in Rust - --> $DIR/overflow_check_conditional.rs:14:8 + --> $DIR/overflow_check_conditional.rs:11:8 | LL | if a < a - b {} | ^^^^^^^^^ From d94ec3091ecc663ef30f288ac859694e2512f31b Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 8 Mar 2023 08:44:35 +0000 Subject: [PATCH 12/13] Bless 32bit. --- tests/mir-opt/const_prop/discriminant.main.ConstProp.32bit.diff | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/mir-opt/const_prop/discriminant.main.ConstProp.32bit.diff b/tests/mir-opt/const_prop/discriminant.main.ConstProp.32bit.diff index b9a10704be0d..6d8738aa61aa 100644 --- a/tests/mir-opt/const_prop/discriminant.main.ConstProp.32bit.diff +++ b/tests/mir-opt/const_prop/discriminant.main.ConstProp.32bit.diff @@ -22,7 +22,7 @@ - switchInt(move _4) -> [1: bb1, otherwise: bb3]; // scope 2 at $DIR/discriminant.rs:+1:21: +1:31 + _3 = const Option::::Some(true); // scope 2 at $DIR/discriminant.rs:+1:34: +1:44 + // mir::Constant -+ // + span: $DIR/discriminant.rs:12:34: 12:44 ++ // + span: no-location + // + literal: Const { ty: Option, val: Value(Scalar(0x01)) } + _4 = const 1_isize; // scope 2 at $DIR/discriminant.rs:+1:21: +1:31 + switchInt(const 1_isize) -> [1: bb1, otherwise: bb3]; // scope 2 at $DIR/discriminant.rs:+1:21: +1:31 From 8179b2e5f8ab3524aa7860d8b34897949dde4b65 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 8 Mar 2023 14:41:37 +0000 Subject: [PATCH 13/13] Remove useless parameter to operand_from_scalar. --- .../rustc_mir_transform/src/const_prop.rs | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_mir_transform/src/const_prop.rs b/compiler/rustc_mir_transform/src/const_prop.rs index 334dd90d4ae4..6c5202549ead 100644 --- a/compiler/rustc_mir_transform/src/const_prop.rs +++ b/compiler/rustc_mir_transform/src/const_prop.rs @@ -458,11 +458,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { scalar, )) = *value { - *operand = self.operand_from_scalar( - scalar, - value.layout.ty, - DUMMY_SP, - ); + *operand = self.operand_from_scalar(scalar, value.layout.ty); } } } @@ -587,9 +583,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } /// Creates a new `Operand::Constant` from a `Scalar` value - fn operand_from_scalar(&self, scalar: Scalar, ty: Ty<'tcx>, span: Span) -> Operand<'tcx> { + fn operand_from_scalar(&self, scalar: Scalar, ty: Ty<'tcx>) -> Operand<'tcx> { Operand::Constant(Box::new(Constant { - span, + span: DUMMY_SP, user_ty: None, literal: ConstantKind::from_scalar(self.tcx, scalar, ty), })) @@ -634,8 +630,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { if let Some(Right(imm)) = imm { match *imm { interpret::Immediate::Scalar(scalar) => { - *rval = - Rvalue::Use(self.operand_from_scalar(scalar, value.layout.ty, DUMMY_SP)); + *rval = Rvalue::Use(self.operand_from_scalar(scalar, value.layout.ty)); } Immediate::ScalarPair(..) => { // Found a value represented as a pair. For now only do const-prop if the type @@ -933,11 +928,7 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> { && self.should_const_prop(value) { trace!("assertion on {:?} should be {:?}", value, expected); - *cond = self.operand_from_scalar( - value_const, - self.tcx.types.bool, - DUMMY_SP, - ); + *cond = self.operand_from_scalar(value_const, self.tcx.types.bool); } } TerminatorKind::SwitchInt { ref mut discr, .. } => {