Skip to content

Commit

Permalink
Generate an intermediate temporary for Drop constants.
Browse files Browse the repository at this point in the history
To limit the fallout from this, don't do this for the last (or only) operand in an rvalue.
  • Loading branch information
oli-obk committed May 4, 2022
1 parent afcd33a commit db02e61
Show file tree
Hide file tree
Showing 8 changed files with 122 additions and 38 deletions.
17 changes: 9 additions & 8 deletions compiler/rustc_mir_build/src/build/expr/as_operand.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! See docs in build/expr/mod.rs

use crate::build::expr::category::Category;
use crate::build::{BlockAnd, BlockAndExtension, Builder};
use crate::build::{BlockAnd, BlockAndExtension, Builder, NeedsTemporary};
use rustc_middle::middle::region;
use rustc_middle::mir::*;
use rustc_middle::thir::*;
Expand All @@ -20,7 +20,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
expr: &Expr<'tcx>,
) -> BlockAnd<Operand<'tcx>> {
let local_scope = self.local_scope();
self.as_operand(block, Some(local_scope), expr, None)
self.as_operand(block, Some(local_scope), expr, None, NeedsTemporary::Maybe)
}

/// Returns an operand suitable for use until the end of the current scope expression and
Expand Down Expand Up @@ -94,32 +94,33 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
///
/// Like `as_local_call_operand`, except that the argument will
/// not be valid once `scope` ends.
#[instrument(level = "debug", skip(self, scope))]
crate fn as_operand(
&mut self,
mut block: BasicBlock,
scope: Option<region::Scope>,
expr: &Expr<'tcx>,
local_info: Option<Box<LocalInfo<'tcx>>>,
needs_temporary: NeedsTemporary,
) -> BlockAnd<Operand<'tcx>> {
debug!("as_operand(block={:?}, expr={:?} local_info={:?})", block, expr, local_info);
let this = self;

if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind {
let source_info = this.source_info(expr.span);
let region_scope = (region_scope, source_info);
return this.in_scope(region_scope, lint_level, |this| {
this.as_operand(block, scope, &this.thir[value], local_info)
this.as_operand(block, scope, &this.thir[value], local_info, needs_temporary)
});
}

let category = Category::of(&expr.kind).unwrap();
debug!("as_operand: category={:?} for={:?}", category, expr.kind);
debug!(?category, ?expr.kind);
match category {
Category::Constant => {
Category::Constant if let NeedsTemporary::No = needs_temporary || !expr.ty.needs_drop(this.tcx, this.param_env) => {
let constant = this.as_constant(expr);
block.and(Operand::Constant(Box::new(constant)))
}
Category::Place | Category::Rvalue(..) => {
Category::Constant | Category::Place | Category::Rvalue(..) => {
let operand = unpack!(block = this.as_temp(block, scope, expr, Mutability::Mut));
if this.local_decls[operand].local_info.is_none() {
this.local_decls[operand].local_info = local_info;
Expand Down Expand Up @@ -176,6 +177,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}

this.as_operand(block, scope, expr, None)
this.as_operand(block, scope, expr, None, NeedsTemporary::Maybe)
}
}
85 changes: 67 additions & 18 deletions compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rustc_index::vec::Idx;

use crate::build::expr::as_place::PlaceBase;
use crate::build::expr::category::{Category, RvalueFunc};
use crate::build::{BlockAnd, BlockAndExtension, Builder};
use crate::build::{BlockAnd, BlockAndExtension, Builder, NeedsTemporary};
use rustc_hir::lang_items::LangItem;
use rustc_middle::middle::region;
use rustc_middle::mir::AssertKind;
Expand Down Expand Up @@ -52,17 +52,28 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
})
}
ExprKind::Repeat { value, count } => {
let value_operand =
unpack!(block = this.as_operand(block, scope, &this.thir[value], None));
let value_operand = unpack!(
block =
this.as_operand(block, scope, &this.thir[value], None, NeedsTemporary::No)
);
block.and(Rvalue::Repeat(value_operand, count))
}
ExprKind::Binary { op, lhs, rhs } => {
let lhs = unpack!(block = this.as_operand(block, scope, &this.thir[lhs], None));
let rhs = unpack!(block = this.as_operand(block, scope, &this.thir[rhs], None));
let lhs = unpack!(
block =
this.as_operand(block, scope, &this.thir[lhs], None, NeedsTemporary::Maybe)
);
let rhs = unpack!(
block =
this.as_operand(block, scope, &this.thir[rhs], None, NeedsTemporary::No)
);
this.build_binary_op(block, op, expr_span, expr.ty, lhs, rhs)
}
ExprKind::Unary { op, arg } => {
let arg = unpack!(block = this.as_operand(block, scope, &this.thir[arg], None));
let arg = unpack!(
block =
this.as_operand(block, scope, &this.thir[arg], None, NeedsTemporary::No)
);
// Check for -MIN on signed integers
if this.check_overflow && op == UnOp::Neg && expr.ty.is_signed() {
let bool_ty = this.tcx.types.bool;
Expand Down Expand Up @@ -167,13 +178,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block.and(Rvalue::Use(Operand::Move(Place::from(result))))
}
ExprKind::Cast { source } => {
let source =
unpack!(block = this.as_operand(block, scope, &this.thir[source], None));
let source = unpack!(
block =
this.as_operand(block, scope, &this.thir[source], None, NeedsTemporary::No)
);
block.and(Rvalue::Cast(CastKind::Misc, source, expr.ty))
}
ExprKind::Pointer { cast, source } => {
let source =
unpack!(block = this.as_operand(block, scope, &this.thir[source], None));
let source = unpack!(
block =
this.as_operand(block, scope, &this.thir[source], None, NeedsTemporary::No)
);
block.and(Rvalue::Cast(CastKind::Pointer(cast), source, expr.ty))
}
ExprKind::Array { ref fields } => {
Expand Down Expand Up @@ -208,7 +223,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fields: Vec<_> = fields
.into_iter()
.copied()
.map(|f| unpack!(block = this.as_operand(block, scope, &this.thir[f], None)))
.map(|f| {
unpack!(
block = this.as_operand(
block,
scope,
&this.thir[f],
None,
NeedsTemporary::Maybe
)
)
})
.collect();

block.and(Rvalue::Aggregate(Box::new(AggregateKind::Array(el_ty)), fields))
Expand All @@ -219,7 +244,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fields: Vec<_> = fields
.into_iter()
.copied()
.map(|f| unpack!(block = this.as_operand(block, scope, &this.thir[f], None)))
.map(|f| {
unpack!(
block = this.as_operand(
block,
scope,
&this.thir[f],
None,
NeedsTemporary::Maybe
)
)
})
.collect();

block.and(Rvalue::Aggregate(Box::new(AggregateKind::Tuple), fields))
Expand Down Expand Up @@ -296,7 +331,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
)
),
_ => {
unpack!(block = this.as_operand(block, scope, upvar, None))
unpack!(
block = this.as_operand(
block,
scope,
upvar,
None,
NeedsTemporary::Maybe
)
)
}
}
}
Expand Down Expand Up @@ -325,13 +368,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
literal: ConstantKind::zero_sized(this.tcx.types.unit),
}))))
}
ExprKind::Yield { .. }
| ExprKind::Literal { .. }

ExprKind::Literal { .. }
| ExprKind::NamedConst { .. }
| ExprKind::NonHirLiteral { .. }
| ExprKind::ConstParam { .. }
| ExprKind::ConstBlock { .. }
| ExprKind::StaticRef { .. }
| ExprKind::StaticRef { .. } => {
let constant = this.as_constant(expr);
block.and(Rvalue::Use(Operand::Constant(Box::new(constant))))
}

ExprKind::Yield { .. }
| ExprKind::Block { .. }
| ExprKind::Match { .. }
| ExprKind::If { .. }
Expand Down Expand Up @@ -359,9 +407,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// so make an operand and then return that
debug_assert!(!matches!(
Category::of(&expr.kind),
Some(Category::Rvalue(RvalueFunc::AsRvalue))
Some(Category::Rvalue(RvalueFunc::AsRvalue) | Category::Constant)
));
let operand = unpack!(block = this.as_operand(block, scope, expr, None));
let operand =
unpack!(block = this.as_operand(block, scope, expr, None, NeedsTemporary::No));
block.and(Rvalue::Use(operand))
}
}
Expand Down
16 changes: 12 additions & 4 deletions compiler/rustc_mir_build/src/build/expr/into.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! See docs in build/expr/mod.rs

use crate::build::expr::category::{Category, RvalueFunc};
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder, NeedsTemporary};
use rustc_ast::InlineAsmOptions;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::stack::ensure_sufficient_stack;
Expand Down Expand Up @@ -329,7 +329,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block,
Some(scope),
&this.thir[f.expr],
Some(local_info)
Some(local_info),
NeedsTemporary::Maybe,
)
),
)
Expand Down Expand Up @@ -516,8 +517,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

ExprKind::Yield { value } => {
let scope = this.local_scope();
let value =
unpack!(block = this.as_operand(block, Some(scope), &this.thir[value], None));
let value = unpack!(
block = this.as_operand(
block,
Some(scope),
&this.thir[value],
None,
NeedsTemporary::No
)
);
let resume = this.cfg.start_new_block();
this.cfg.terminate(
block,
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_mir_build/src/build/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,18 @@ rustc_index::newtype_index! {
struct ScopeId { .. }
}

#[derive(Debug)]
enum NeedsTemporary {
/// Use this variant when whatever you are converting with `as_operand`
/// is the last thing you are converting. This means that if we introduced
/// an intermediate temporary, we'd only read it immediately after, so we can
/// also avoid it.
No,
/// For all cases where you aren't sure or that are too expensive to compute
/// for now. It is always safe to fall back to this.
Maybe,
}

///////////////////////////////////////////////////////////////////////////
/// The `BlockAnd` "monad" packages up the new basic block along with a
/// produced value (sometimes just unit, of course). The `unpack!`
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_mir_build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#![feature(box_patterns)]
#![feature(control_flow_enum)]
#![feature(crate_visibility_modifier)]
#![feature(if_let_guard)]
#![feature(let_chains)]
#![feature(let_else)]
#![feature(min_specialization)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
scope 2 {
}
+ scope 3 (inlined Vec::<u32>::new) { // at $DIR/inline-into-box-place.rs:8:33: 8:43
+ let mut _8: alloc::raw_vec::RawVec<u32>; // in scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ }

bb0: {
Expand All @@ -34,8 +35,8 @@
- (*_5) = Vec::<u32>::new() -> [return: bb2, unwind: bb5]; // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43
+ StorageLive(_7); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43
+ _7 = &mut (*_5); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43
+ Deinit((*_7)); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ ((*_7).0: alloc::raw_vec::RawVec<u32>) = const alloc::raw_vec::RawVec::<u32> { ptr: Unique::<u32> { pointer: NonNull::<u32> { pointer: {0x4 as *const u32} }, _marker: PhantomData::<u32> }, cap: 0_usize, alloc: std::alloc::Global }; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ StorageLive(_8); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ _8 = const alloc::raw_vec::RawVec::<u32> { ptr: Unique::<u32> { pointer: NonNull::<u32> { pointer: {0x4 as *const u32} }, _marker: PhantomData::<u32> }, cap: 0_usize, alloc: std::alloc::Global }; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
// mir::Constant
- // + span: $DIR/inline-into-box-place.rs:8:33: 8:41
- // + user_ty: UserType(1)
Expand All @@ -46,7 +47,10 @@
+ // + span: $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ // + user_ty: UserType(0)
+ // + literal: Const { ty: alloc::raw_vec::RawVec<u32>, val: Value(ByRef { alloc: Allocation { bytes: [4, 0, 0, 0, 0, 0, 0, 0], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [255], len: Size { raw: 8 } }, align: Align { pow2: 2 }, mutability: Not, extra: () }, offset: Size { raw: 0 } }) }
+ Deinit((*_7)); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ ((*_7).0: alloc::raw_vec::RawVec<u32>) = move _8; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ ((*_7).1: usize) = const 0_usize; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ StorageDead(_8); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ StorageDead(_7); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43
_1 = move _5; // scope 0 at $DIR/inline-into-box-place.rs:8:29: 8:43
StorageDead(_5); // scope 0 at $DIR/inline-into-box-place.rs:8:42: 8:43
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
scope 2 {
}
+ scope 3 (inlined Vec::<u32>::new) { // at $DIR/inline-into-box-place.rs:8:33: 8:43
+ let mut _8: alloc::raw_vec::RawVec<u32>; // in scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ }

bb0: {
Expand All @@ -34,8 +35,8 @@
- (*_5) = Vec::<u32>::new() -> [return: bb2, unwind: bb5]; // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43
+ StorageLive(_7); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43
+ _7 = &mut (*_5); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43
+ Deinit((*_7)); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ ((*_7).0: alloc::raw_vec::RawVec<u32>) = const alloc::raw_vec::RawVec::<u32> { ptr: Unique::<u32> { pointer: NonNull::<u32> { pointer: {0x4 as *const u32} }, _marker: PhantomData::<u32> }, cap: 0_usize, alloc: std::alloc::Global }; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ StorageLive(_8); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ _8 = const alloc::raw_vec::RawVec::<u32> { ptr: Unique::<u32> { pointer: NonNull::<u32> { pointer: {0x4 as *const u32} }, _marker: PhantomData::<u32> }, cap: 0_usize, alloc: std::alloc::Global }; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
// mir::Constant
- // + span: $DIR/inline-into-box-place.rs:8:33: 8:41
- // + user_ty: UserType(1)
Expand All @@ -46,7 +47,10 @@
+ // + span: $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ // + user_ty: UserType(0)
+ // + literal: Const { ty: alloc::raw_vec::RawVec<u32>, val: Value(ByRef { alloc: Allocation { bytes: [4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], relocations: Relocations(SortedMap { data: [] }), init_mask: InitMask { blocks: [65535], len: Size { raw: 16 } }, align: Align { pow2: 3 }, mutability: Not, extra: () }, offset: Size { raw: 0 } }) }
+ Deinit((*_7)); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ ((*_7).0: alloc::raw_vec::RawVec<u32>) = move _8; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ ((*_7).1: usize) = const 0_usize; // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ StorageDead(_8); // scope 3 at $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
+ StorageDead(_7); // scope 0 at $DIR/inline-into-box-place.rs:8:33: 8:43
_1 = move _5; // scope 0 at $DIR/inline-into-box-place.rs:8:29: 8:43
StorageDead(_5); // scope 0 at $DIR/inline-into-box-place.rs:8:42: 8:43
Expand Down
13 changes: 9 additions & 4 deletions src/test/ui/consts/issue-90762.rs
Original file line number Diff line number Diff line change
@@ -1,26 +1,31 @@
// run-pass
#![allow(unreachable_code)]

use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::atomic::{AtomicBool, Ordering, AtomicUsize};

struct Print(usize);

impl Drop for Print {
fn drop(&mut self) {
println!("{}", self.0);
FOO[self.0].store(true, Ordering::Relaxed);
assert_eq!(BAR.fetch_sub(1, Ordering::Relaxed), self.0);
}
}

const A: Print = Print(0);
const B: Print = Print(1);

static FOO: [AtomicBool; 3] = [AtomicBool::new(false), AtomicBool::new(false), AtomicBool::new(false)];
static FOO: [AtomicBool; 3] =
[AtomicBool::new(false), AtomicBool::new(false), AtomicBool::new(false)];
static BAR: AtomicUsize = AtomicUsize::new(2);

fn main() {
loop {
std::mem::forget(({A}, B, Print(2), break));
std::mem::forget(({ A }, B, Print(2), break));
}
for (i, b) in FOO.iter().enumerate() {
assert!(b.load(Ordering::Relaxed), "{} not set", i);
}
}
assert_eq!(BAR.fetch_add(1, Ordering::Relaxed), usize::max_value());
}

0 comments on commit db02e61

Please sign in to comment.