Skip to content

[MIR] Get rid of that nasty unit_ty temporary lval #30635

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.

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

Merged
merged 2 commits into from
Jan 12, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 10 additions & 5 deletions src/librustc_mir/build/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use build::{BlockAnd, Builder};
use build::{BlockAnd, BlockAndExtension, Builder};
use hair::*;
use rustc::mir::repr::*;
use rustc_front::hir;
Expand All @@ -19,11 +19,16 @@ impl<'a,'tcx> Builder<'a,'tcx> {
mut block: BasicBlock,
ast_block: &'tcx hir::Block)
-> BlockAnd<()> {
let this = self;
let Block { extent, span: _, stmts, expr } = this.hir.mirror(ast_block);
this.in_scope(extent, block, |this| {
let Block { extent, span, stmts, expr } = self.hir.mirror(ast_block);
self.in_scope(extent, block, move |this| {
unpack!(block = this.stmts(block, stmts));
this.into(destination, block, expr)
match expr {
Some(expr) => this.into(destination, block, expr),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need the impl of EvalInto for Option<ExprRef<'tcx>>? I suspect not. In any case I think perhaps we ought to remove it, since it's ambiguous what should happen on None.

None => {
this.cfg.push_assign_unit(block, span, destination);
block.unit()
}
}
})
}
}
25 changes: 17 additions & 8 deletions src/librustc_mir/build/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,6 @@ impl<'tcx> CFG<'tcx> {
self.block_data_mut(block).statements.push(statement);
}

pub fn push_assign_constant(&mut self,
block: BasicBlock,
span: Span,
temp: &Lvalue<'tcx>,
constant: Constant<'tcx>) {
self.push_assign(block, span, temp, Rvalue::Use(Operand::Constant(constant)));
}

pub fn push_drop(&mut self, block: BasicBlock, span: Span,
kind: DropKind, lvalue: &Lvalue<'tcx>) {
self.push(block, Statement {
Expand All @@ -64,6 +56,23 @@ impl<'tcx> CFG<'tcx> {
});
}

pub fn push_assign_constant(&mut self,
block: BasicBlock,
span: Span,
temp: &Lvalue<'tcx>,
constant: Constant<'tcx>) {
self.push_assign(block, span, temp, Rvalue::Use(Operand::Constant(constant)));
}

pub fn push_assign_unit(&mut self,
block: BasicBlock,
span: Span,
lvalue: &Lvalue<'tcx>) {
self.push_assign(block, span, lvalue, Rvalue::Aggregate(
AggregateKind::Tuple, vec![]
));
}

pub fn terminate(&mut self,
block: BasicBlock,
terminator: Terminator<'tcx>) {
Expand Down
32 changes: 26 additions & 6 deletions src/librustc_mir/build/expr/into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,14 @@ impl<'a,'tcx> Builder<'a,'tcx> {
});

unpack!(then_block = this.into(destination, then_block, then_expr));
unpack!(else_block = this.into(destination, else_block, else_expr));
else_block = if let Some(else_expr) = else_expr {
unpack!(this.into(destination, else_block, else_expr))
} else {
// Body of the `if` expression without an `else` clause must return `()`, thus
// we implicitly generate a `else {}` if it is not specified.
this.cfg.push_assign_unit(else_block, expr_span, &Lvalue::ReturnPointer);
else_block
};

let join_block = this.cfg.start_new_block();
this.cfg.terminate(then_block, Terminator::Goto { target: join_block });
Expand Down Expand Up @@ -157,11 +164,18 @@ impl<'a,'tcx> Builder<'a,'tcx> {
}

// execute the body, branching back to the test
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add at least a comment here explaining that we are (ab)using destination to serve as a dummy location

Copy link
Contributor

Choose a reason for hiding this comment

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

and that we think this is ok because (a) the type of destination should always be () and (b) the destination will still be considered uninitialized for the first iteration, so it should not cause anything to type-check that wouldn't otherwise

let unit_temp = this.unit_temp.clone();
let body_block_end = unpack!(this.into(&unit_temp, body_block, body));
// We write body’s “return value” into the destination of loop. This is fine,
// because:
//
// * In Rust both loop expression and its body are required to have `()`
// as the “return value”;
// * The destination will be considered uninitialised (given it was
// uninitialised before the loop) during the first iteration, thus
// disallowing its use inside the body. Alternatively, if it was already
// initialised, the `destination` can only possibly have a value of `()`,
// therefore, “mutating” the destination during iteration is fine.
let body_block_end = unpack!(this.into(destination, body_block, body));
this.cfg.terminate(body_block_end, Terminator::Goto { target: loop_block });

// final point is exit_block
exit_block.unit()
})
}
Expand Down Expand Up @@ -206,7 +220,13 @@ impl<'a,'tcx> Builder<'a,'tcx> {
this.break_or_continue(expr_span, label, block, |loop_scope| loop_scope.break_block)
}
ExprKind::Return { value } => {
unpack!(block = this.into(&Lvalue::ReturnPointer, block, value));
block = match value {
Some(value) => unpack!(this.into(&Lvalue::ReturnPointer, block, value)),
None => {
this.cfg.push_assign_unit(block, expr_span, &Lvalue::ReturnPointer);
block
}
};
let extent = this.extent_of_outermost_scope();
this.exit_scope(expr_span, extent, block, END_BLOCK);
this.cfg.start_new_block().unit()
Expand Down
15 changes: 1 addition & 14 deletions src/librustc_mir/build/into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
//! wrapped up as expressions (e.g. blocks). To make this ergonomic, we use this
//! latter `EvalInto` trait.

use build::{BlockAnd, BlockAndExtension, Builder};
use build::{BlockAnd, Builder};
use hair::*;
use rustc::mir::repr::*;

Expand Down Expand Up @@ -58,16 +58,3 @@ impl<'tcx> EvalInto<'tcx> for Expr<'tcx> {
builder.into_expr(destination, block, self)
}
}

impl<'tcx> EvalInto<'tcx> for Option<ExprRef<'tcx>> {
fn eval_into<'a>(self,
builder: &mut Builder<'a, 'tcx>,
destination: &Lvalue<'tcx>,
block: BasicBlock)
-> BlockAnd<()> {
match self {
Some(expr) => builder.into(destination, block, expr),
None => block.unit(),
}
}
}
11 changes: 2 additions & 9 deletions src/librustc_mir/build/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ pub struct Builder<'a, 'tcx: 'a> {
cfg: CFG<'tcx>,
scopes: Vec<scope::Scope<'tcx>>,
loop_scopes: Vec<scope::LoopScope>,
unit_temp: Lvalue<'tcx>,
var_decls: Vec<VarDecl<'tcx>>,
var_indices: FnvHashMap<ast::NodeId, u32>,
temp_decls: Vec<TempDecl<'tcx>>,
Expand Down Expand Up @@ -79,7 +78,7 @@ macro_rules! unpack {
///////////////////////////////////////////////////////////////////////////
// construct() -- the main entry point for building MIR for a function

pub fn construct<'a,'tcx>(mut hir: Cx<'a,'tcx>,
pub fn construct<'a,'tcx>(hir: Cx<'a,'tcx>,
_span: Span,
implicit_arguments: Vec<Ty<'tcx>>,
explicit_arguments: Vec<(Ty<'tcx>, &'tcx hir::Pat)>,
Expand All @@ -89,20 +88,14 @@ pub fn construct<'a,'tcx>(mut hir: Cx<'a,'tcx>,
-> Mir<'tcx> {
let cfg = CFG { basic_blocks: vec![] };

// it's handy to have a temporary of type `()` sometimes, so make
// one from the start and keep it available
let temp_decls = vec![TempDecl::<'tcx> { ty: hir.unit_ty() }];
let unit_temp = Lvalue::Temp(0);

let mut builder = Builder {
hir: hir,
cfg: cfg,
scopes: vec![],
loop_scopes: vec![],
temp_decls: temp_decls,
temp_decls: vec![],
var_decls: vec![],
var_indices: FnvHashMap(),
unit_temp: unit_temp,
};

assert_eq!(builder.cfg.start_new_block(), START_BLOCK);
Expand Down
4 changes: 0 additions & 4 deletions src/librustc_mir/hair/cx/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,6 @@ impl<'a,'tcx:'a> Cx<'a, 'tcx> {
ast.make_mirror(self)
}

pub fn unit_ty(&mut self) -> Ty<'tcx> {
self.tcx.mk_nil()
}

pub fn usize_ty(&mut self) -> Ty<'tcx> {
self.tcx.types.usize
}
Expand Down