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

Clean up MIR drop generation #61872

Merged
merged 6 commits into from
Jun 26, 2019
+536 −468
Merged

Clean up MIR drop generation #61872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
62bec71
Fix incorrect double assignment in MIR for while loops
matthewjasper Jun 15, 2019
101a2f5
Use `as_temp` to evaluate statement expressions
matthewjasper Jun 15, 2019
82a1a89
Avoid checking if references implement drop
matthewjasper Jun 15, 2019
be23bd4
Unify `return`, `break` and `continue` handling
matthewjasper Jun 15, 2019
b86e675
Add StorageDead statements for `while` conditions
matthewjasper Jun 15, 2019
3131427
Use `Local`s instead of `Place`s in MIR drop generation
matthewjasper Jun 15, 2019
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
4 changes: 2 additions & 2 deletions src/librustc_mir/build/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

let source_info = this.source_info(span);
for stmt in stmts {
let Stmt { kind, opt_destruction_scope, span: stmt_span } = this.hir.mirror(stmt);
let Stmt { kind, opt_destruction_scope } = this.hir.mirror(stmt);
match kind {
StmtKind::Expr { scope, expr } => {
this.block_context.push(BlockFrame::Statement { ignores_expr_result: true });
Expand All @@ -87,7 +87,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let si = (scope, source_info);
this.in_scope(si, LintLevel::Inherited, |this| {
let expr = this.hir.mirror(expr);
this.stmt_expr(block, expr, Some(stmt_span))
this.stmt_expr(block, expr, Some(scope))
})
}));
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/build/expr/as_rvalue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.schedule_drop_storage_and_value(
expr_span,
scope,
&Place::from(result),
result,
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
value.ty,
);
}
Expand Down Expand Up @@ -559,7 +559,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.schedule_drop_storage_and_value(
upvar_span,
temp_lifetime,
&Place::from(temp),
temp,
upvar_ty,
);
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/build/expr/as_temp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.schedule_drop(
expr_span,
temp_lifetime,
temp_place,
temp,
expr_ty,
DropKind::Storage,
);
Expand All @@ -101,7 +101,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.schedule_drop(
expr_span,
temp_lifetime,
temp_place,
temp,
expr_ty,
DropKind::Value,
);
Expand Down
23 changes: 11 additions & 12 deletions src/librustc_mir/build/expr/into.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,20 +179,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// conduct the test, if necessary
let body_block;
if let Some(cond_expr) = opt_cond_expr {
Copy link
Contributor

Choose a reason for hiding this comment

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

N.B. As part of implementing while a && let b = c { ... } I'll make a PR to remove hir::ExprKind::While and then hair::ExprKind::Loop will presumably need to drop it's condition field and so the else branch would be floated out and the if branch would be removed... I hope the changes here do not cause problems for this...?

Copy link
Contributor Author

@matthewjasper matthewjasper Jun 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That should be fine, this is only a problem because while loops are special cased. For any sensible HIR lowering this won't be a problem. The temporary will probably end up storage-live for the whole loop body, but that isn't observable. The following two functions generate almost identical optimized MIR after this PR, for example.

fn while_loop(c: bool) {
    while get_bool(c) {
        if get_bool(c) {
            break;
        }
    }
}

// What the above should probably expand to
fn exp_while_loop(c: bool) {
    loop {
        match get_bool(c) {
            true => {
                {if get_bool(c) {
                    break;
                }}
                continue;
            }
            _ => {}
        }
        break;
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool.

For any sensible HIR lowering this won't be a problem.

Specifically, the HIR lowering should likely be:

'label: while $cond $block

==>

'label: loop {
    match DropTemps($cond) {
        true => $block,
        _ => break,
    }
}

(is there a particular reason you are using continue; and an empty block?)

Copy link
Contributor Author

@matthewjasper matthewjasper Jun 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's slightly closer to what the RFC specified. What you're suggesting is probably better.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be unnecessary now with #61988.

let loop_block_end;
let cond = unpack!(
loop_block_end = this.as_local_operand(loop_block, cond_expr)
);
body_block = this.cfg.start_new_block();
let term =
TerminatorKind::if_(this.hir.tcx(), cond, body_block, exit_block);
this.cfg.terminate(loop_block_end, source_info, term);
let cond_expr = this.hir.mirror(cond_expr);
let (true_block, false_block)
= this.test_bool(loop_block, cond_expr, source_info);
body_block = true_block;

// if the test is false, there's no `break` to assign `destination`, so
// we have to do it; this overwrites any `break`-assigned value but it's
// always `()` anyway
this.cfg
.push_assign_unit(exit_block, source_info, destination);
// we have to do it
this.cfg.push_assign_unit(false_block, source_info, destination);
this.cfg.terminate(
false_block,
source_info,
TerminatorKind::Goto { target: exit_block },
);
} else {
body_block = this.cfg.start_new_block();
let diverge_cleanup = this.diverge_cleanup();
Expand Down
145 changes: 39 additions & 106 deletions src/librustc_mir/build/expr/stmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
use crate::build::scope::BreakableScope;
use crate::build::scope::BreakableTarget;
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
use crate::hair::*;
use rustc::middle::region;
use rustc::mir::*;

impl<'a, 'tcx> Builder<'a, 'tcx> {
/// Builds a block of MIR statements to evaluate the HAIR `expr`.
/// If the original expression was an AST statement,
/// (e.g., `some().code(&here());`) then `opt_stmt_span` is the
/// span of that statement (including its semicolon, if any).
/// Diagnostics use this span (which may be larger than that of
/// `expr`) to identify when statement temporaries are dropped.
pub fn stmt_expr(&mut self,
mut block: BasicBlock,
expr: Expr<'tcx>,
opt_stmt_span: Option<StatementSpan>)
-> BlockAnd<()>
{
/// The scope is used if a statement temporary must be dropped.
pub fn stmt_expr(
&mut self,
mut block: BasicBlock,
expr: Expr<'tcx>,
statement_scope: Option<region::Scope>,
) -> BlockAnd<()> {
let this = self;
let expr_span = expr.span;
let source_info = this.source_info(expr.span);
Expand All @@ -30,7 +30,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
} => {
let value = this.hir.mirror(value);
this.in_scope((region_scope, source_info), lint_level, |this| {
this.stmt_expr(block, value, opt_stmt_span)
this.stmt_expr(block, value, statement_scope)
})
}
ExprKind::Assign { lhs, rhs } => {
Expand Down Expand Up @@ -98,70 +98,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block.unit()
}
ExprKind::Continue { label } => {
let BreakableScope {
continue_block,
region_scope,
..
} = *this.find_breakable_scope(expr_span, label);
let continue_block = continue_block
.expect("Attempted to continue in non-continuable breakable block");
this.exit_scope(
expr_span,
(region_scope, source_info),
block,
continue_block,
);
this.cfg.start_new_block().unit()
this.break_scope(block, None, BreakableTarget::Continue(label), source_info)
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
}
ExprKind::Break { label, value } => {
let (break_block, region_scope, destination) = {
let BreakableScope {
break_block,
region_scope,
ref break_destination,
..
} = *this.find_breakable_scope(expr_span, label);
(break_block, region_scope, break_destination.clone())
};
if let Some(value) = value {
debug!("stmt_expr Break val block_context.push(SubExpr) : {:?}", expr2);
this.block_context.push(BlockFrame::SubExpr);
unpack!(block = this.into(&destination, block, value));
this.block_context.pop();
} else {
this.cfg.push_assign_unit(block, source_info, &destination)
}
this.exit_scope(expr_span, (region_scope, source_info), block, break_block);
this.cfg.start_new_block().unit()
this.break_scope(block, value, BreakableTarget::Break(label), source_info)
}
ExprKind::Return { value } => {
block = match value {
Some(value) => {
debug!("stmt_expr Return val block_context.push(SubExpr) : {:?}", expr2);
this.block_context.push(BlockFrame::SubExpr);
let result = unpack!(
this.into(
&Place::RETURN_PLACE,
block,
value
)
);
this.block_context.pop();
result
}
None => {
this.cfg.push_assign_unit(
block,
source_info,
&Place::RETURN_PLACE,
);
block
}
};
let region_scope = this.region_scope_of_return_scope();
let return_block = this.return_block();
this.exit_scope(expr_span, (region_scope, source_info), block, return_block);
this.cfg.start_new_block().unit()
this.break_scope(block, value, BreakableTarget::Return, source_info)
}
ExprKind::InlineAsm {
asm,
Expand Down Expand Up @@ -199,7 +142,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block.unit()
}
_ => {
let expr_ty = expr.ty;
assert!(
statement_scope.is_some(),
"Should not be calling `stmt_expr` on a general expression \
without a statement scope",
);

// Issue #54382: When creating temp for the value of
// expression like:
Expand All @@ -208,48 +155,34 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
//
// it is usually better to focus on `the_value` rather
// than the entirety of block(s) surrounding it.
let mut temp_span = expr_span;
let mut temp_in_tail_of_block = false;
if let ExprKind::Block { body } = expr.kind {
if let Some(tail_expr) = &body.expr {
let mut expr = tail_expr;
while let rustc::hir::ExprKind::Block(subblock, _label) = &expr.node {
if let Some(subtail_expr) = &subblock.expr {
expr = subtail_expr
} else {
break;
let adjusted_span = (|| {
if let ExprKind::Block { body } = expr.kind {
if let Some(tail_expr) = &body.expr {
let mut expr = tail_expr;
while let rustc::hir::ExprKind::Block(subblock, _label) = &expr.node {
if let Some(subtail_expr) = &subblock.expr {
expr = subtail_expr
} else {
break;
}
}
}
temp_span = expr.span;
temp_in_tail_of_block = true;
}
}

let temp = {
let mut local_decl = LocalDecl::new_temp(expr.ty.clone(), temp_span);
if temp_in_tail_of_block {
if this.block_context.currently_ignores_tail_results() {
local_decl = local_decl.block_tail(BlockTailInfo {
this.block_context.push(BlockFrame::TailExpr {
tail_result_is_ignored: true
});
return Some(expr.span);
}
}
let temp = this.local_decls.push(local_decl);
let place = Place::from(temp);
debug!("created temp {:?} for expr {:?} in block_context: {:?}",
temp, expr, this.block_context);
place
};
unpack!(block = this.into(&temp, block, expr));
None
})();

// Attribute drops of the statement's temps to the
// semicolon at the statement's end.
let drop_point = this.hir.tcx().sess.source_map().end_point(match opt_stmt_span {
None => expr_span,
Some(StatementSpan(span)) => span,
});
let temp = unpack!(block =
this.as_temp(block, statement_scope, expr, Mutability::Not));
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved

if let Some(span) = adjusted_span {
this.local_decls[temp].source_info.span = span;
this.block_context.pop();
}

unpack!(block = this.build_drop(block, drop_point, temp, expr_ty));
block.unit()
}
}
Expand Down
Loading