Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix constants not getting dropped if part of a diverging expression #94775

Merged
merged 2 commits into from
May 4, 2022
Merged

Fix constants not getting dropped if part of a diverging expression #94775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
afcd33a
Add regression test
oli-obk Mar 9, 2022
db02e61
Generate an intermediate temporary for `Drop` constants.
oli-obk Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions compiler/rustc_mir_build/src/build/expr/as_operand.rs
View file
Open in desktop
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) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The perf regression is definitely real and likely due

  • to changes in how as_operand is being inlined due to this change
  • directly from this change, as internals of the needs_drop check show up in benchmarks now.

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
8 changes: 6 additions & 2 deletions src/test/mir-opt/inline/inline_into_box_place.main.Inline.32bit.diff
View file
Open in desktop
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
8 changes: 6 additions & 2 deletions src/test/mir-opt/inline/inline_into_box_place.main.Inline.64bit.diff
View file
Open in desktop
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
31 changes: 31 additions & 0 deletions src/test/ui/consts/issue-90762.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// run-pass
#![allow(unreachable_code)]

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 BAR: AtomicUsize = AtomicUsize::new(2);

fn main() {
loop {
std::mem::forget(({ A }, B, Print(2), break));
}
for (i, b) in FOO.iter().enumerate() {
assert!(b.load(Ordering::Relaxed), "{} not set", i);
}
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(BAR.fetch_add(1, Ordering::Relaxed), usize::max_value());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a load should be sufficient here, shouldn't it?

}