Skip to content

Commit

Permalink
Auto merge of #74821 - oli-obk:const_eval_read_uninit_fast_path, r=we…
Browse files Browse the repository at this point in the history 
…sleywiser

Check whether locals are too large instead of whether accesses into them are too large

Essentially this stops const prop from attempting to optimize

```rust
let mut x = [0_u8; 5000];
x[42] = 3;
```

I don't expect this to be a perf improvement without #73656 (which is also where the lack of this PR will be a perf regression).

r? @wesleywiser
  • Loading branch information
@bors
bors committed Aug 7, 2020
2 parents 64f99b4 + 1864a97 commit 4d43423
Show file tree
Hide file tree
Showing 8 changed files with 266 additions and 90 deletions.
130 changes: 70 additions & 60 deletions src/librustc_mir/transform/const_prop.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ use crate::interpret::{
};
use crate::transform::{MirPass, MirSource};

/// The maximum number of bytes that we'll allocate space for a return value.
/// The maximum number of bytes that we'll allocate space for a local or the return value.
/// Needed for #66397, because otherwise we eval into large places and that can cause OOM or just
/// Severely regress performance.
const MAX_ALLOC_LIMIT: u64 = 1024;

/// Macro for machine-specific `InterpError` without allocation.
Expand Down Expand Up @@ -155,14 +157,19 @@ struct ConstPropMachine<'mir, 'tcx> {
written_only_inside_own_block_locals: FxHashSet<Local>,
/// Locals that need to be cleared after every block terminates.
only_propagate_inside_block_locals: BitSet<Local>,
can_const_prop: IndexVec<Local, ConstPropMode>,
}

impl<'mir, 'tcx> ConstPropMachine<'mir, 'tcx> {
fn new(only_propagate_inside_block_locals: BitSet<Local>) -> Self {
fn new(
only_propagate_inside_block_locals: BitSet<Local>,
can_const_prop: IndexVec<Local, ConstPropMode>,
) -> Self {
Self {
stack: Vec::new(),
written_only_inside_own_block_locals: Default::default(),
only_propagate_inside_block_locals,
can_const_prop,
}
}
}
Expand Down Expand Up @@ -241,6 +248,9 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
local: Local,
) -> InterpResult<'tcx, Result<&'a mut LocalValue<Self::PointerTag>, MemPlace<Self::PointerTag>>>
{
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) {
ecx.machine.written_only_inside_own_block_locals.insert(local);
}
Expand Down Expand Up @@ -285,7 +295,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
struct ConstPropagator<'mir, 'tcx> {
ecx: InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>,
tcx: TyCtxt<'tcx>,
can_const_prop: IndexVec<Local, ConstPropMode>,
param_env: ParamEnv<'tcx>,
// FIXME(eddyb) avoid cloning these two fields more than once,
// by accessing them through `ecx` instead.
Expand Down Expand Up @@ -331,7 +340,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
let param_env = tcx.param_env_reveal_all_normalized(def_id);

let span = tcx.def_span(def_id);
let can_const_prop = CanConstProp::check(body);
// FIXME: `CanConstProp::check` computes the layout of all locals, return those layouts
// so we can write them to `ecx.frame_mut().locals.layout, reducing the duplication in
// `layout_of` query invocations.
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 {
Expand All @@ -342,7 +354,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
tcx,
span,
param_env,
ConstPropMachine::new(only_propagate_inside_block_locals),
ConstPropMachine::new(only_propagate_inside_block_locals, can_const_prop),
(),
);

Expand All @@ -368,7 +380,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
ecx,
tcx,
param_env,
can_const_prop,
// FIXME(eddyb) avoid cloning these two fields more than once,
// by accessing them through `ecx` instead.
source_scopes: body.source_scopes.clone(),
Expand Down Expand Up @@ -612,15 +623,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
fn const_prop(
&mut self,
rvalue: &Rvalue<'tcx>,
place_layout: TyAndLayout<'tcx>,
source_info: SourceInfo,
place: Place<'tcx>,
) -> Option<()> {
// #66397: Don't try to eval into large places as that can cause an OOM
if place_layout.size >= Size::from_bytes(MAX_ALLOC_LIMIT) {
return None;
}

// 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
Expand Down Expand Up @@ -893,7 +898,11 @@ struct CanConstProp {

impl CanConstProp {
/// Returns true if `local` can be propagated
fn check(body: &Body<'_>) -> IndexVec<Local, ConstPropMode> {
fn check(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
body: &Body<'tcx>,
) -> IndexVec<Local, ConstPropMode> {
let mut cpv = CanConstProp {
can_const_prop: IndexVec::from_elem(ConstPropMode::FullConstProp, &body.local_decls),
found_assignment: BitSet::new_empty(body.local_decls.len()),
Expand All @@ -903,6 +912,16 @@ impl CanConstProp {
),
};
for (local, val) in cpv.can_const_prop.iter_enumerated_mut() {
let ty = body.local_decls[local].ty;
match tcx.layout_of(param_env.and(ty)) {
Ok(layout) if layout.size < Size::from_bytes(MAX_ALLOC_LIMIT) => {}
// Either the layout fails to compute, then we can't use this local anyway
// or the local is too large, then we don't want to.
_ => {
*val = ConstPropMode::NoPropagation;
continue;
}
}
// Cannot use args at all
// Cannot use locals because if x < y { y - x } else { x - y } would
// lint for x != y
Expand Down Expand Up @@ -1018,61 +1037,52 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
let source_info = statement.source_info;
self.source_info = Some(source_info);
if let StatementKind::Assign(box (place, ref mut rval)) = statement.kind {
let place_ty: Ty<'tcx> = place.ty(&self.local_decls, self.tcx).ty;
if let Ok(place_layout) = self.tcx.layout_of(self.param_env.and(place_ty)) {
let can_const_prop = self.can_const_prop[place.local];
if let Some(()) = self.const_prop(rval, place_layout, source_info, 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(value) = self.get_const(place) {
if 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);
}
let can_const_prop = self.ecx.machine.can_const_prop[place.local];
if let Some(()) = self.const_prop(rval, source_info, 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(value) = self.get_const(place) {
if 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::OnlyPropagateInto | ConstPropMode::NoPropagation => {
trace!("can't propagate into {:?}", place);
if place.local != RETURN_PLACE {
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 undefined
// ```
// 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);
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 undefined
// ```
// FIXME: we overzealously erase the entire local, because that's easier to
// implement.
trace!(
"cannot propagate into {:?}, because the type of the local is generic.",
"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);
Expand Down
85 changes: 85 additions & 0 deletions src/test/mir-opt/const_prop/large_array_index.main.ConstProp.diff.32bit
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
- // MIR for `main` before ConstProp
+ // MIR for `main` after ConstProp

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/large_array_index.rs:4:11: 4:11
let _1: u8; // in scope 0 at $DIR/large_array_index.rs:6:9: 6:10
let mut _2: [u8; 5000]; // in scope 0 at $DIR/large_array_index.rs:6:17: 6:29
let _3: usize; // in scope 0 at $DIR/large_array_index.rs:6:30: 6:31
let mut _4: usize; // in scope 0 at $DIR/large_array_index.rs:6:17: 6:32
let mut _5: bool; // in scope 0 at $DIR/large_array_index.rs:6:17: 6:32
scope 1 {
debug x => _1; // in scope 1 at $DIR/large_array_index.rs:6:9: 6:10
}

bb0: {
StorageLive(_1); // scope 0 at $DIR/large_array_index.rs:6:9: 6:10
StorageLive(_2); // scope 0 at $DIR/large_array_index.rs:6:17: 6:29
_2 = [const 0_u8; 5000]; // scope 0 at $DIR/large_array_index.rs:6:17: 6:29
// ty::Const
// + ty: u8
// + val: Value(Scalar(0x00))
// mir::Constant
// + span: $DIR/large_array_index.rs:6:18: 6:22
// + literal: Const { ty: u8, val: Value(Scalar(0x00)) }
StorageLive(_3); // scope 0 at $DIR/large_array_index.rs:6:30: 6:31
_3 = const 2_usize; // scope 0 at $DIR/large_array_index.rs:6:30: 6:31
// ty::Const
// + ty: usize
// + val: Value(Scalar(0x00000002))
// mir::Constant
// + span: $DIR/large_array_index.rs:6:30: 6:31
// + literal: Const { ty: usize, val: Value(Scalar(0x00000002)) }
_4 = const 5000_usize; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
// ty::Const
// + ty: usize
// + val: Value(Scalar(0x00001388))
// mir::Constant
// + span: $DIR/large_array_index.rs:6:17: 6:32
// + literal: Const { ty: usize, val: Value(Scalar(0x00001388)) }
- _5 = Lt(_3, _4); // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
- assert(move _5, "index out of bounds: the len is {} but the index is {}", move _4, _3) -> bb1; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
+ _5 = const true; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/large_array_index.rs:6:17: 6:32
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
+ assert(const true, "index out of bounds: the len is {} but the index is {}", const 5000_usize, const 2_usize) -> bb1; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/large_array_index.rs:6:17: 6:32
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
+ // ty::Const
+ // + ty: usize
+ // + val: Value(Scalar(0x00001388))
+ // mir::Constant
+ // + span: $DIR/large_array_index.rs:6:17: 6:32
+ // + literal: Const { ty: usize, val: Value(Scalar(0x00001388)) }
+ // ty::Const
+ // + ty: usize
+ // + val: Value(Scalar(0x00000002))
+ // mir::Constant
+ // + span: $DIR/large_array_index.rs:6:17: 6:32
+ // + literal: Const { ty: usize, val: Value(Scalar(0x00000002)) }
}

bb1: {
_1 = _2[_3]; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
StorageDead(_3); // scope 0 at $DIR/large_array_index.rs:6:32: 6:33
StorageDead(_2); // scope 0 at $DIR/large_array_index.rs:6:32: 6:33
_0 = const (); // scope 0 at $DIR/large_array_index.rs:4:11: 7:2
// ty::Const
// + ty: ()
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/large_array_index.rs:4:11: 7:2
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
StorageDead(_1); // scope 0 at $DIR/large_array_index.rs:7:1: 7:2
return; // scope 0 at $DIR/large_array_index.rs:7:2: 7:2
}
}

Loading

0 comments on commit 4d43423

Please sign in to comment.