From f242fe3c04621dca3aaa3ab288a3812b45633ffd Mon Sep 17 00:00:00 2001 From: James Miller Date: Fri, 15 Apr 2016 12:36:16 +1200 Subject: [PATCH 1/9] Various improvements to MIR and LLVM IR Construction Primarily affects the MIR construction, which indirectly improves LLVM IR generation, but some LLVM IR changes have been made too. * Handle "statement expressions" more intelligently. These are expressions that always evaluate to `()`. Previously a temporary would be generated as a destination to translate into, which is unnecessary. This affects assignment, augmented assignment, `return`, `break` and `continue`. * Avoid inserting drops for non-drop types in more places. Scheduled drops were already skipped for types that we knew wouldn't need dropping at construction time. However manually-inserted drops like those for `x` in `x = y;` were still generated. `build_drop` now takes a type parameter like its `schedule_drop` counterpart and checks to see if the type needs dropping. * Avoid generating an extra temporary for an assignment where the types involved don't need dropping. Previously an expression like `a = b + 1;` would result in a temporary for `b + 1`. This is so the RHS can be evaluated, then the LHS evaluated and dropped and have everything work correctly. However, this isn't necessary if the `LHS` doesn't need a drop, as we can just overwrite the existing value. * Improves lvalue analysis to allow treating an `Rvalue::Use` as an operand in certain conditions. The reason for it never being an operand is so it can be zeroed/drop-filled, but this is only true for types that need dropping. The first two changes result in significantly fewer MIR blocks being generated, as previously almost every statement would end up generating a new block due to the drop of the `()` temporary being generated. --- src/librustc_mir/build/block.rs | 123 ++++++++++++++++++++++++++-- src/librustc_mir/build/expr/into.rs | 87 ++------------------ src/librustc_mir/build/expr/mod.rs | 2 +- src/librustc_mir/build/scope.rs | 7 +- src/librustc_trans/mir/analyze.rs | 21 +++-- src/librustc_trans/mir/mod.rs | 11 ++- src/librustc_trans/mir/rvalue.rs | 32 ++++++-- src/librustc_trans/mir/statement.rs | 19 ++++- 8 files changed, 195 insertions(+), 107 deletions(-) diff --git a/src/librustc_mir/build/block.rs b/src/librustc_mir/build/block.rs index 8c98408e2390a..fc67bc7ded083 100644 --- a/src/librustc_mir/build/block.rs +++ b/src/librustc_mir/build/block.rs @@ -9,9 +9,12 @@ // except according to those terms. use build::{BlockAnd, BlockAndExtension, Builder}; +use build::scope::LoopScope; use hair::*; +use rustc::middle::region::CodeExtent; use rustc::mir::repr::*; use rustc::hir; +use syntax::codemap::Span; impl<'a,'tcx> Builder<'a,'tcx> { pub fn ast_block(&mut self, @@ -44,11 +47,7 @@ impl<'a,'tcx> Builder<'a,'tcx> { StmtKind::Expr { scope, expr } => { unpack!(block = this.in_scope(scope, block, |this, _| { let expr = this.hir.mirror(expr); - let expr_span = expr.span; - let temp = this.temp(expr.ty.clone()); - unpack!(block = this.into(&temp, block, expr)); - unpack!(block = this.build_drop(block, expr_span, temp)); - block.unit() + this.stmt_expr(block, expr) })); } StmtKind::Let { remainder_scope, init_scope, pattern, initializer } => { @@ -83,4 +82,118 @@ impl<'a,'tcx> Builder<'a,'tcx> { block.unit() }) } + + pub fn stmt_expr(&mut self, mut block: BasicBlock, expr: Expr<'tcx>) -> BlockAnd<()> { + let this = self; + let expr_span = expr.span; + let scope_id = this.innermost_scope_id(); + // Handle a number of expressions that don't need a destination at all. This + // avoids needing a mountain of temporary `()` variables. + match expr.kind { + ExprKind::Scope { extent, value } => { + let value = this.hir.mirror(value); + this.in_scope(extent, block, |this, _| this.stmt_expr(block, value)) + } + ExprKind::Assign { lhs, rhs } => { + let lhs = this.hir.mirror(lhs); + let scope_id = this.innermost_scope_id(); + let lhs_span = lhs.span; + let lhs_ty = lhs.ty; + + let lhs_needs_drop = this.hir.needs_drop(lhs_ty); + + // Note: we evaluate assignments right-to-left. This + // is better for borrowck interaction with overloaded + // operators like x[j] = x[i]. + + // Generate better code for things that don't need to be + // dropped. We need the temporary as_operand generates + // so we can clean up the data if evaluating the LHS unwinds, + // but if the LHS (and therefore the RHS) doesn't need + // unwinding, we just translate directly to an rvalue instead. + let rhs = if lhs_needs_drop { + let op = unpack!(block = this.as_operand(block, rhs)); + Rvalue::Use(op) + } else { + unpack!(block = this.as_rvalue(block, rhs)) + }; + + let lhs = unpack!(block = this.as_lvalue(block, lhs)); + unpack!(block = this.build_drop(block, lhs_span, lhs.clone(), lhs_ty)); + this.cfg.push_assign(block, scope_id, expr_span, &lhs, rhs); + block.unit() + } + ExprKind::AssignOp { op, lhs, rhs } => { + // FIXME(#28160) there is an interesting semantics + // question raised here -- should we "freeze" the + // value of the lhs here? I'm inclined to think not, + // since it seems closer to the semantics of the + // overloaded version, which takes `&mut self`. This + // only affects weird things like `x += {x += 1; x}` + // -- is that equal to `x + (x + 1)` or `2*(x+1)`? + + // As above, RTL. + let rhs = unpack!(block = this.as_operand(block, rhs)); + let lhs = unpack!(block = this.as_lvalue(block, lhs)); + + // we don't have to drop prior contents or anything + // because AssignOp is only legal for Copy types + // (overloaded ops should be desugared into a call). + this.cfg.push_assign(block, scope_id, expr_span, &lhs, + Rvalue::BinaryOp(op, + Operand::Consume(lhs.clone()), + rhs)); + + block.unit() + } + ExprKind::Continue { label } => { + this.break_or_continue(expr_span, label, block, + |loop_scope| loop_scope.continue_block) + } + ExprKind::Break { label } => { + this.break_or_continue(expr_span, label, block, |loop_scope| { + loop_scope.might_break = true; + loop_scope.break_block + }) + } + ExprKind::Return { value } => { + block = match value { + Some(value) => unpack!(this.into(&Lvalue::ReturnPointer, block, value)), + None => { + this.cfg.push_assign_unit(block, scope_id, + expr_span, &Lvalue::ReturnPointer); + block + } + }; + let extent = this.extent_of_return_scope(); + let return_block = this.return_block(); + this.exit_scope(expr_span, extent, block, return_block); + this.cfg.start_new_block().unit() + } + _ => { + let expr_span = expr.span; + let expr_ty = expr.ty; + let temp = this.temp(expr.ty.clone()); + unpack!(block = this.into(&temp, block, expr)); + unpack!(block = this.build_drop(block, expr_span, temp, expr_ty)); + block.unit() + } + } + } + + fn break_or_continue(&mut self, + span: Span, + label: Option, + block: BasicBlock, + exit_selector: F) + -> BlockAnd<()> + where F: FnOnce(&mut LoopScope) -> BasicBlock + { + let (exit_block, extent) = { + let loop_scope = self.find_loop_scope(span, label); + (exit_selector(loop_scope), loop_scope.extent) + }; + self.exit_scope(span, extent, block, exit_block); + self.cfg.start_new_block().unit() + } } diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index fe32f1de0c520..b7729b01737ba 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -12,12 +12,9 @@ use build::{BlockAnd, BlockAndExtension, Builder}; use build::expr::category::{Category, RvalueFunc}; -use build::scope::LoopScope; use hair::*; -use rustc::middle::region::CodeExtent; use rustc::ty; use rustc::mir::repr::*; -use syntax::codemap::Span; impl<'a,'tcx> Builder<'a,'tcx> { /// Compile `expr`, storing the result into `destination`, which @@ -207,65 +204,6 @@ impl<'a,'tcx> Builder<'a,'tcx> { } exit_block.unit() } - ExprKind::Assign { lhs, rhs } => { - // Note: we evaluate assignments right-to-left. This - // is better for borrowck interaction with overloaded - // operators like x[j] = x[i]. - let lhs = this.hir.mirror(lhs); - let lhs_span = lhs.span; - let rhs = unpack!(block = this.as_operand(block, rhs)); - let lhs = unpack!(block = this.as_lvalue(block, lhs)); - unpack!(block = this.build_drop(block, lhs_span, lhs.clone())); - this.cfg.push_assign(block, scope_id, expr_span, &lhs, Rvalue::Use(rhs)); - block.unit() - } - ExprKind::AssignOp { op, lhs, rhs } => { - // FIXME(#28160) there is an interesting semantics - // question raised here -- should we "freeze" the - // value of the lhs here? I'm inclined to think not, - // since it seems closer to the semantics of the - // overloaded version, which takes `&mut self`. This - // only affects weird things like `x += {x += 1; x}` - // -- is that equal to `x + (x + 1)` or `2*(x+1)`? - - // As above, RTL. - let rhs = unpack!(block = this.as_operand(block, rhs)); - let lhs = unpack!(block = this.as_lvalue(block, lhs)); - - // we don't have to drop prior contents or anything - // because AssignOp is only legal for Copy types - // (overloaded ops should be desugared into a call). - this.cfg.push_assign(block, scope_id, expr_span, &lhs, - Rvalue::BinaryOp(op, - Operand::Consume(lhs.clone()), - rhs)); - - block.unit() - } - ExprKind::Continue { label } => { - this.break_or_continue(expr_span, label, block, - |loop_scope| loop_scope.continue_block) - } - ExprKind::Break { label } => { - this.break_or_continue(expr_span, label, block, |loop_scope| { - loop_scope.might_break = true; - loop_scope.break_block - }) - } - ExprKind::Return { value } => { - block = match value { - Some(value) => unpack!(this.into(&Lvalue::ReturnPointer, block, value)), - None => { - this.cfg.push_assign_unit(block, scope_id, - expr_span, &Lvalue::ReturnPointer); - block - } - }; - let extent = this.extent_of_return_scope(); - let return_block = this.return_block(); - this.exit_scope(expr_span, extent, block, return_block); - this.cfg.start_new_block().unit() - } ExprKind::Call { ty, fun, args } => { let diverges = match ty.sty { ty::TyFnDef(_, _, ref f) | ty::TyFnPtr(ref f) => { @@ -294,6 +232,15 @@ impl<'a,'tcx> Builder<'a,'tcx> { success.unit() } + // These cases don't actually need a destination + ExprKind::Assign { .. } | + ExprKind::AssignOp { .. } | + ExprKind::Continue { .. } | + ExprKind::Break { .. } | + ExprKind::Return {.. } => { + this.stmt_expr(block, expr) + } + // these are the cases that are more naturally handled by some other mode ExprKind::Unary { .. } | ExprKind::Binary { .. } | @@ -327,20 +274,4 @@ impl<'a,'tcx> Builder<'a,'tcx> { } } } - - fn break_or_continue(&mut self, - span: Span, - label: Option, - block: BasicBlock, - exit_selector: F) - -> BlockAnd<()> - where F: FnOnce(&mut LoopScope) -> BasicBlock - { - let (exit_block, extent) = { - let loop_scope = self.find_loop_scope(span, label); - (exit_selector(loop_scope), loop_scope.extent) - }; - self.exit_scope(span, extent, block, exit_block); - self.cfg.start_new_block().unit() - } } diff --git a/src/librustc_mir/build/expr/mod.rs b/src/librustc_mir/build/expr/mod.rs index 0f168f307aa01..b131b0ad5c11d 100644 --- a/src/librustc_mir/build/expr/mod.rs +++ b/src/librustc_mir/build/expr/mod.rs @@ -75,5 +75,5 @@ mod as_lvalue; mod as_rvalue; mod as_operand; mod as_temp; -mod category; +pub mod category; mod into; diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index bfbf27d46d005..95c931df9e5cc 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -497,8 +497,11 @@ impl<'a,'tcx> Builder<'a,'tcx> { pub fn build_drop(&mut self, block: BasicBlock, span: Span, - value: Lvalue<'tcx>) - -> BlockAnd<()> { + value: Lvalue<'tcx>, + ty: Ty<'tcx>) -> BlockAnd<()> { + if !self.hir.needs_drop(ty) { + return block.unit(); + } let scope_id = self.innermost_scope_id(); let next_target = self.cfg.start_new_block(); let diverge_target = self.diverge_cleanup(); diff --git a/src/librustc_trans/mir/analyze.rs b/src/librustc_trans/mir/analyze.rs index f721e88a95413..6730f8c0fcc79 100644 --- a/src/librustc_trans/mir/analyze.rs +++ b/src/librustc_trans/mir/analyze.rs @@ -20,7 +20,7 @@ use super::rvalue; pub fn lvalue_temps<'bcx,'tcx>(bcx: Block<'bcx,'tcx>, mir: &mir::Mir<'tcx>) -> BitVector { - let mut analyzer = TempAnalyzer::new(mir.temp_decls.len()); + let mut analyzer = TempAnalyzer::new(mir, bcx, mir.temp_decls.len()); analyzer.visit_mir(mir); @@ -30,7 +30,8 @@ pub fn lvalue_temps<'bcx,'tcx>(bcx: Block<'bcx,'tcx>, if ty.is_scalar() || ty.is_unique() || ty.is_region_ptr() || - ty.is_simd() + ty.is_simd() || + common::type_is_zero_size(bcx.ccx(), ty) { // These sorts of types are immediates that we can store // in an ValueRef without an alloca. @@ -50,14 +51,20 @@ pub fn lvalue_temps<'bcx,'tcx>(bcx: Block<'bcx,'tcx>, analyzer.lvalue_temps } -struct TempAnalyzer { +struct TempAnalyzer<'mir, 'bcx, 'tcx: 'mir + 'bcx> { + mir: &'mir mir::Mir<'tcx>, + bcx: Block<'bcx, 'tcx>, lvalue_temps: BitVector, seen_assigned: BitVector } -impl TempAnalyzer { - fn new(temp_count: usize) -> TempAnalyzer { +impl<'mir, 'bcx, 'tcx> TempAnalyzer<'mir, 'bcx, 'tcx> { + fn new(mir: &'mir mir::Mir<'tcx>, + bcx: Block<'bcx, 'tcx>, + temp_count: usize) -> TempAnalyzer<'mir, 'bcx, 'tcx> { TempAnalyzer { + mir: mir, + bcx: bcx, lvalue_temps: BitVector::new(temp_count), seen_assigned: BitVector::new(temp_count) } @@ -75,7 +82,7 @@ impl TempAnalyzer { } } -impl<'tcx> Visitor<'tcx> for TempAnalyzer { +impl<'mir, 'bcx, 'tcx> Visitor<'tcx> for TempAnalyzer<'mir, 'bcx, 'tcx> { fn visit_assign(&mut self, block: mir::BasicBlock, lvalue: &mir::Lvalue<'tcx>, @@ -85,7 +92,7 @@ impl<'tcx> Visitor<'tcx> for TempAnalyzer { match *lvalue { mir::Lvalue::Temp(index) => { self.mark_assigned(index as usize); - if !rvalue::rvalue_creates_operand(rvalue) { + if !rvalue::rvalue_creates_operand(self.mir, self.bcx, rvalue) { self.mark_as_lvalue(index as usize); } } diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index 7ec3c9345be58..a5e116ae7b626 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -34,7 +34,7 @@ use rustc_data_structures::bitvec::BitVector; use self::lvalue::{LvalueRef, get_dataptr, get_meta}; use rustc_mir::traversal; -use self::operand::OperandRef; +use self::operand::{OperandRef, OperandValue}; #[derive(Clone)] pub enum CachedMir<'mir, 'tcx: 'mir> { @@ -150,6 +150,15 @@ pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) { TempRef::Lvalue(LvalueRef::alloca(&bcx, mty, &format!("temp{:?}", i))) + } else if common::type_is_zero_size(bcx.ccx(), mty) { + // Zero-size temporaries aren't always initialized, which + // doesn't matter because they don't contain data, but + // we need something in the operand. + let op = OperandRef { + val: OperandValue::Immediate(common::C_nil(bcx.ccx())), + ty: mty + }; + TempRef::Operand(Some(op)) } else { // If this is an immediate temp, we do not create an // alloca in advance. Instead we wait until we see the diff --git a/src/librustc_trans/mir/rvalue.rs b/src/librustc_trans/mir/rvalue.rs index 641603f5aaad9..3050d2c5290b7 100644 --- a/src/librustc_trans/mir/rvalue.rs +++ b/src/librustc_trans/mir/rvalue.rs @@ -18,7 +18,7 @@ use rustc::mir::repr as mir; use asm; use base; use callee::Callee; -use common::{self, C_uint, BlockAndBuilder, Result}; +use common::{self, C_uint, Block, BlockAndBuilder, Result}; use datum::{Datum, Lvalue}; use debuginfo::DebugLoc; use declare; @@ -29,6 +29,7 @@ use type_of; use tvec; use value::Value; use Disr; +use glue; use super::MirContext; use super::operand::{OperandRef, OperandValue}; @@ -217,7 +218,9 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { } _ => { - assert!(rvalue_creates_operand(rvalue)); + bcx.with_block(|bcx| { + assert!(rvalue_creates_operand(&self.mir, bcx, rvalue)); + }); let (bcx, temp) = self.trans_rvalue_operand(bcx, rvalue, debug_loc); self.store_operand(&bcx, dest.llval, temp); bcx @@ -231,7 +234,10 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { debug_loc: DebugLoc) -> (BlockAndBuilder<'bcx, 'tcx>, OperandRef<'tcx>) { - assert!(rvalue_creates_operand(rvalue), "cannot trans {:?} to operand", rvalue); + bcx.with_block(|bcx| { + assert!(rvalue_creates_operand(&self.mir, bcx, rvalue), + "cannot trans {:?} to operand", rvalue); + }); match *rvalue { mir::Rvalue::Cast(ref kind, ref source, cast_ty) => { @@ -483,7 +489,10 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { (bcx, operand) } - mir::Rvalue::Use(..) | + mir::Rvalue::Use(ref operand) => { + let operand = self.trans_operand(&bcx, operand); + (bcx, operand) + } mir::Rvalue::Repeat(..) | mir::Rvalue::Aggregate(..) | mir::Rvalue::Slice { .. } | @@ -599,7 +608,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { } } -pub fn rvalue_creates_operand<'tcx>(rvalue: &mir::Rvalue<'tcx>) -> bool { +pub fn rvalue_creates_operand<'bcx, 'tcx>(mir: &mir::Mir<'tcx>, bcx: Block<'bcx, 'tcx>, + rvalue: &mir::Rvalue<'tcx>) -> bool { match *rvalue { mir::Rvalue::Ref(..) | mir::Rvalue::Len(..) | @@ -608,16 +618,20 @@ pub fn rvalue_creates_operand<'tcx>(rvalue: &mir::Rvalue<'tcx>) -> bool { mir::Rvalue::UnaryOp(..) | mir::Rvalue::Box(..) => true, - mir::Rvalue::Use(..) | // (**) mir::Rvalue::Repeat(..) | mir::Rvalue::Aggregate(..) | mir::Rvalue::Slice { .. } | mir::Rvalue::InlineAsm { .. } => false, + mir::Rvalue::Use(ref operand) => { + let ty = mir.operand_ty(bcx.tcx(), operand); + let ty = bcx.monomorphize(&ty); + // Types that don't need dropping can just be an operand, + // this allows temporary lvalues, used as rvalues, to + // avoid a stack slot when it's unnecessary + !glue::type_needs_drop(bcx.tcx(), ty) + } } // (*) this is only true if the type is suitable - // (**) we need to zero-out the source operand after moving, so we are restricted to either - // ensuring all users of `Use` zero it out themselves or not allowing to “create” operand for - // it. } diff --git a/src/librustc_trans/mir/statement.rs b/src/librustc_trans/mir/statement.rs index e4967cead07e9..6ec4ee3ce9326 100644 --- a/src/librustc_trans/mir/statement.rs +++ b/src/librustc_trans/mir/statement.rs @@ -9,7 +9,7 @@ // except according to those terms. use rustc::mir::repr as mir; -use common::BlockAndBuilder; +use common::{self, BlockAndBuilder}; use debuginfo::DebugLoc; use super::MirContext; @@ -42,9 +42,20 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { bcx } TempRef::Operand(Some(_)) => { - span_bug!(statement.span, - "operand {:?} already assigned", - rvalue); + let ty = self.mir.lvalue_ty(bcx.tcx(), lvalue); + let ty = bcx.monomorphize(&ty.to_ty(bcx.tcx())); + + if !common::type_is_zero_size(bcx.ccx(), ty) { + span_bug!(statement.span, + "operand {:?} already assigned", + rvalue); + } else { + // If the type is zero-sized, it's already been set here, + // but we still need to make sure we translate the operand + let (bcx, _) = self.trans_rvalue_operand(bcx, rvalue, + debug_loc); + bcx + } } } } From c2de80f05fe32bdb694c085b48a87727175e3691 Mon Sep 17 00:00:00 2001 From: James Miller Date: Sat, 16 Apr 2016 17:38:18 +1200 Subject: [PATCH 2/9] Address comments Moves `stmt_expr` into its own module, `expr::stmt`. --- src/librustc_mir/build/block.rs | 117 ------------------------ src/librustc_mir/build/expr/mod.rs | 3 +- src/librustc_mir/build/expr/stmt.rs | 135 ++++++++++++++++++++++++++++ src/librustc_trans/mir/analyze.rs | 14 +-- src/librustc_trans/mir/mod.rs | 17 ++-- src/librustc_trans/mir/rvalue.rs | 15 ++-- src/librustc_trans/mir/statement.rs | 4 +- 7 files changed, 163 insertions(+), 142 deletions(-) create mode 100644 src/librustc_mir/build/expr/stmt.rs diff --git a/src/librustc_mir/build/block.rs b/src/librustc_mir/build/block.rs index fc67bc7ded083..49029f9642e08 100644 --- a/src/librustc_mir/build/block.rs +++ b/src/librustc_mir/build/block.rs @@ -9,12 +9,9 @@ // except according to those terms. use build::{BlockAnd, BlockAndExtension, Builder}; -use build::scope::LoopScope; use hair::*; -use rustc::middle::region::CodeExtent; use rustc::mir::repr::*; use rustc::hir; -use syntax::codemap::Span; impl<'a,'tcx> Builder<'a,'tcx> { pub fn ast_block(&mut self, @@ -82,118 +79,4 @@ impl<'a,'tcx> Builder<'a,'tcx> { block.unit() }) } - - pub fn stmt_expr(&mut self, mut block: BasicBlock, expr: Expr<'tcx>) -> BlockAnd<()> { - let this = self; - let expr_span = expr.span; - let scope_id = this.innermost_scope_id(); - // Handle a number of expressions that don't need a destination at all. This - // avoids needing a mountain of temporary `()` variables. - match expr.kind { - ExprKind::Scope { extent, value } => { - let value = this.hir.mirror(value); - this.in_scope(extent, block, |this, _| this.stmt_expr(block, value)) - } - ExprKind::Assign { lhs, rhs } => { - let lhs = this.hir.mirror(lhs); - let scope_id = this.innermost_scope_id(); - let lhs_span = lhs.span; - let lhs_ty = lhs.ty; - - let lhs_needs_drop = this.hir.needs_drop(lhs_ty); - - // Note: we evaluate assignments right-to-left. This - // is better for borrowck interaction with overloaded - // operators like x[j] = x[i]. - - // Generate better code for things that don't need to be - // dropped. We need the temporary as_operand generates - // so we can clean up the data if evaluating the LHS unwinds, - // but if the LHS (and therefore the RHS) doesn't need - // unwinding, we just translate directly to an rvalue instead. - let rhs = if lhs_needs_drop { - let op = unpack!(block = this.as_operand(block, rhs)); - Rvalue::Use(op) - } else { - unpack!(block = this.as_rvalue(block, rhs)) - }; - - let lhs = unpack!(block = this.as_lvalue(block, lhs)); - unpack!(block = this.build_drop(block, lhs_span, lhs.clone(), lhs_ty)); - this.cfg.push_assign(block, scope_id, expr_span, &lhs, rhs); - block.unit() - } - ExprKind::AssignOp { op, lhs, rhs } => { - // FIXME(#28160) there is an interesting semantics - // question raised here -- should we "freeze" the - // value of the lhs here? I'm inclined to think not, - // since it seems closer to the semantics of the - // overloaded version, which takes `&mut self`. This - // only affects weird things like `x += {x += 1; x}` - // -- is that equal to `x + (x + 1)` or `2*(x+1)`? - - // As above, RTL. - let rhs = unpack!(block = this.as_operand(block, rhs)); - let lhs = unpack!(block = this.as_lvalue(block, lhs)); - - // we don't have to drop prior contents or anything - // because AssignOp is only legal for Copy types - // (overloaded ops should be desugared into a call). - this.cfg.push_assign(block, scope_id, expr_span, &lhs, - Rvalue::BinaryOp(op, - Operand::Consume(lhs.clone()), - rhs)); - - block.unit() - } - ExprKind::Continue { label } => { - this.break_or_continue(expr_span, label, block, - |loop_scope| loop_scope.continue_block) - } - ExprKind::Break { label } => { - this.break_or_continue(expr_span, label, block, |loop_scope| { - loop_scope.might_break = true; - loop_scope.break_block - }) - } - ExprKind::Return { value } => { - block = match value { - Some(value) => unpack!(this.into(&Lvalue::ReturnPointer, block, value)), - None => { - this.cfg.push_assign_unit(block, scope_id, - expr_span, &Lvalue::ReturnPointer); - block - } - }; - let extent = this.extent_of_return_scope(); - let return_block = this.return_block(); - this.exit_scope(expr_span, extent, block, return_block); - this.cfg.start_new_block().unit() - } - _ => { - let expr_span = expr.span; - let expr_ty = expr.ty; - let temp = this.temp(expr.ty.clone()); - unpack!(block = this.into(&temp, block, expr)); - unpack!(block = this.build_drop(block, expr_span, temp, expr_ty)); - block.unit() - } - } - } - - fn break_or_continue(&mut self, - span: Span, - label: Option, - block: BasicBlock, - exit_selector: F) - -> BlockAnd<()> - where F: FnOnce(&mut LoopScope) -> BasicBlock - { - let (exit_block, extent) = { - let loop_scope = self.find_loop_scope(span, label); - (exit_selector(loop_scope), loop_scope.extent) - }; - self.exit_scope(span, extent, block, exit_block); - self.cfg.start_new_block().unit() - } } diff --git a/src/librustc_mir/build/expr/mod.rs b/src/librustc_mir/build/expr/mod.rs index b131b0ad5c11d..17b34f4586e8b 100644 --- a/src/librustc_mir/build/expr/mod.rs +++ b/src/librustc_mir/build/expr/mod.rs @@ -75,5 +75,6 @@ mod as_lvalue; mod as_rvalue; mod as_operand; mod as_temp; -pub mod category; +mod category; mod into; +mod stmt; diff --git a/src/librustc_mir/build/expr/stmt.rs b/src/librustc_mir/build/expr/stmt.rs new file mode 100644 index 0000000000000..3c1672b919751 --- /dev/null +++ b/src/librustc_mir/build/expr/stmt.rs @@ -0,0 +1,135 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use build::{BlockAnd, BlockAndExtension, Builder}; +use build::scope::LoopScope; +use hair::*; +use rustc::middle::region::CodeExtent; +use rustc::mir::repr::*; +use syntax::codemap::Span; + +impl<'a,'tcx> Builder<'a,'tcx> { + + pub fn stmt_expr(&mut self, mut block: BasicBlock, expr: Expr<'tcx>) -> BlockAnd<()> { + let this = self; + let expr_span = expr.span; + let scope_id = this.innermost_scope_id(); + // Handle a number of expressions that don't need a destination at all. This + // avoids needing a mountain of temporary `()` variables. + match expr.kind { + ExprKind::Scope { extent, value } => { + let value = this.hir.mirror(value); + this.in_scope(extent, block, |this, _| this.stmt_expr(block, value)) + } + ExprKind::Assign { lhs, rhs } => { + let lhs = this.hir.mirror(lhs); + let rhs = this.hir.mirror(rhs); + let scope_id = this.innermost_scope_id(); + let lhs_span = lhs.span; + + let lhs_ty = lhs.ty; + let rhs_ty = rhs.ty; + + let lhs_needs_drop = this.hir.needs_drop(lhs_ty); + let rhs_needs_drop = this.hir.needs_drop(rhs_ty); + + // Note: we evaluate assignments right-to-left. This + // is better for borrowck interaction with overloaded + // operators like x[j] = x[i]. + + // Generate better code for things that don't need to be + // dropped. + let rhs = if lhs_needs_drop || rhs_needs_drop { + let op = unpack!(block = this.as_operand(block, rhs)); + Rvalue::Use(op) + } else { + unpack!(block = this.as_rvalue(block, rhs)) + }; + + let lhs = unpack!(block = this.as_lvalue(block, lhs)); + unpack!(block = this.build_drop(block, lhs_span, lhs.clone(), lhs_ty)); + this.cfg.push_assign(block, scope_id, expr_span, &lhs, rhs); + block.unit() + } + ExprKind::AssignOp { op, lhs, rhs } => { + // FIXME(#28160) there is an interesting semantics + // question raised here -- should we "freeze" the + // value of the lhs here? I'm inclined to think not, + // since it seems closer to the semantics of the + // overloaded version, which takes `&mut self`. This + // only affects weird things like `x += {x += 1; x}` + // -- is that equal to `x + (x + 1)` or `2*(x+1)`? + + // As above, RTL. + let rhs = unpack!(block = this.as_operand(block, rhs)); + let lhs = unpack!(block = this.as_lvalue(block, lhs)); + + // we don't have to drop prior contents or anything + // because AssignOp is only legal for Copy types + // (overloaded ops should be desugared into a call). + this.cfg.push_assign(block, scope_id, expr_span, &lhs, + Rvalue::BinaryOp(op, + Operand::Consume(lhs.clone()), + rhs)); + + block.unit() + } + ExprKind::Continue { label } => { + this.break_or_continue(expr_span, label, block, + |loop_scope| loop_scope.continue_block) + } + ExprKind::Break { label } => { + this.break_or_continue(expr_span, label, block, |loop_scope| { + loop_scope.might_break = true; + loop_scope.break_block + }) + } + ExprKind::Return { value } => { + block = match value { + Some(value) => unpack!(this.into(&Lvalue::ReturnPointer, block, value)), + None => { + this.cfg.push_assign_unit(block, scope_id, + expr_span, &Lvalue::ReturnPointer); + block + } + }; + let extent = this.extent_of_return_scope(); + let return_block = this.return_block(); + this.exit_scope(expr_span, extent, block, return_block); + this.cfg.start_new_block().unit() + } + _ => { + let expr_span = expr.span; + let expr_ty = expr.ty; + let temp = this.temp(expr.ty.clone()); + unpack!(block = this.into(&temp, block, expr)); + unpack!(block = this.build_drop(block, expr_span, temp, expr_ty)); + block.unit() + } + } + } + + fn break_or_continue(&mut self, + span: Span, + label: Option, + block: BasicBlock, + exit_selector: F) + -> BlockAnd<()> + where F: FnOnce(&mut LoopScope) -> BasicBlock + { + let (exit_block, extent) = { + let loop_scope = self.find_loop_scope(span, label); + (exit_selector(loop_scope), loop_scope.extent) + }; + self.exit_scope(span, extent, block, exit_block); + self.cfg.start_new_block().unit() + } + +} diff --git a/src/librustc_trans/mir/analyze.rs b/src/librustc_trans/mir/analyze.rs index 6730f8c0fcc79..0b88ba554da67 100644 --- a/src/librustc_trans/mir/analyze.rs +++ b/src/librustc_trans/mir/analyze.rs @@ -14,13 +14,13 @@ use rustc_data_structures::bitvec::BitVector; use rustc::mir::repr as mir; use rustc::mir::visit::{Visitor, LvalueContext}; -use common::{self, Block}; +use common::{self, Block, BlockAndBuilder}; use super::rvalue; pub fn lvalue_temps<'bcx,'tcx>(bcx: Block<'bcx,'tcx>, - mir: &mir::Mir<'tcx>) - -> BitVector { - let mut analyzer = TempAnalyzer::new(mir, bcx, mir.temp_decls.len()); + mir: &mir::Mir<'tcx>) -> BitVector { + let bcx = bcx.build(); + let mut analyzer = TempAnalyzer::new(mir, &bcx, mir.temp_decls.len()); analyzer.visit_mir(mir); @@ -51,16 +51,16 @@ pub fn lvalue_temps<'bcx,'tcx>(bcx: Block<'bcx,'tcx>, analyzer.lvalue_temps } -struct TempAnalyzer<'mir, 'bcx, 'tcx: 'mir + 'bcx> { +struct TempAnalyzer<'mir, 'bcx: 'mir, 'tcx: 'bcx> { mir: &'mir mir::Mir<'tcx>, - bcx: Block<'bcx, 'tcx>, + bcx: &'mir BlockAndBuilder<'bcx, 'tcx>, lvalue_temps: BitVector, seen_assigned: BitVector } impl<'mir, 'bcx, 'tcx> TempAnalyzer<'mir, 'bcx, 'tcx> { fn new(mir: &'mir mir::Mir<'tcx>, - bcx: Block<'bcx, 'tcx>, + bcx: &'mir BlockAndBuilder<'bcx, 'tcx>, temp_count: usize) -> TempAnalyzer<'mir, 'bcx, 'tcx> { TempAnalyzer { mir: mir, diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index a5e116ae7b626..b7661ce039ca3 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -108,6 +108,16 @@ enum TempRef<'tcx> { Operand(Option>), } +impl<'tcx> TempRef<'tcx> { + fn new_operand(val: OperandValue, ty: ty::Ty<'tcx>) -> TempRef<'tcx> { + let op = OperandRef { + val: val, + ty: ty + }; + TempRef::Operand(Some(op)) + } +} + /////////////////////////////////////////////////////////////////////////// pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) { @@ -154,11 +164,8 @@ pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) { // Zero-size temporaries aren't always initialized, which // doesn't matter because they don't contain data, but // we need something in the operand. - let op = OperandRef { - val: OperandValue::Immediate(common::C_nil(bcx.ccx())), - ty: mty - }; - TempRef::Operand(Some(op)) + let val = OperandValue::Immediate(common::C_nil(bcx.ccx())); + TempRef::new_operand(val, mty) } else { // If this is an immediate temp, we do not create an // alloca in advance. Instead we wait until we see the diff --git a/src/librustc_trans/mir/rvalue.rs b/src/librustc_trans/mir/rvalue.rs index 3050d2c5290b7..67d7f44cbbf41 100644 --- a/src/librustc_trans/mir/rvalue.rs +++ b/src/librustc_trans/mir/rvalue.rs @@ -18,7 +18,7 @@ use rustc::mir::repr as mir; use asm; use base; use callee::Callee; -use common::{self, C_uint, Block, BlockAndBuilder, Result}; +use common::{self, C_uint, BlockAndBuilder, Result}; use datum::{Datum, Lvalue}; use debuginfo::DebugLoc; use declare; @@ -218,9 +218,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { } _ => { - bcx.with_block(|bcx| { - assert!(rvalue_creates_operand(&self.mir, bcx, rvalue)); - }); + assert!(rvalue_creates_operand(&self.mir, &bcx, rvalue)); let (bcx, temp) = self.trans_rvalue_operand(bcx, rvalue, debug_loc); self.store_operand(&bcx, dest.llval, temp); bcx @@ -234,10 +232,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { debug_loc: DebugLoc) -> (BlockAndBuilder<'bcx, 'tcx>, OperandRef<'tcx>) { - bcx.with_block(|bcx| { - assert!(rvalue_creates_operand(&self.mir, bcx, rvalue), - "cannot trans {:?} to operand", rvalue); - }); + assert!(rvalue_creates_operand(&self.mir, &bcx, rvalue), + "cannot trans {:?} to operand", rvalue); match *rvalue { mir::Rvalue::Cast(ref kind, ref source, cast_ty) => { @@ -608,7 +604,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { } } -pub fn rvalue_creates_operand<'bcx, 'tcx>(mir: &mir::Mir<'tcx>, bcx: Block<'bcx, 'tcx>, +pub fn rvalue_creates_operand<'bcx, 'tcx>(mir: &mir::Mir<'tcx>, + bcx: &BlockAndBuilder<'bcx, 'tcx>, rvalue: &mir::Rvalue<'tcx>) -> bool { match *rvalue { mir::Rvalue::Ref(..) | diff --git a/src/librustc_trans/mir/statement.rs b/src/librustc_trans/mir/statement.rs index 6ec4ee3ce9326..c9a4e540fa06b 100644 --- a/src/librustc_trans/mir/statement.rs +++ b/src/librustc_trans/mir/statement.rs @@ -52,9 +52,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { } else { // If the type is zero-sized, it's already been set here, // but we still need to make sure we translate the operand - let (bcx, _) = self.trans_rvalue_operand(bcx, rvalue, - debug_loc); - bcx + self.trans_rvalue_operand(bcx, rvalue, debug_loc).0 } } } From 869172305f66ba11565c90f9276328c479d5b082 Mon Sep 17 00:00:00 2001 From: James Miller Date: Sat, 16 Apr 2016 17:39:29 +1200 Subject: [PATCH 3/9] Fixup tests The drop glue for `i8` is no longer generated as a trans item --- .../item-collection/impl-in-non-instantiated-generic.rs | 2 -- src/test/codegen-units/item-collection/non-generic-closures.rs | 2 -- src/test/codegen-units/item-collection/statics-and-consts.rs | 2 -- 3 files changed, 6 deletions(-) diff --git a/src/test/codegen-units/item-collection/impl-in-non-instantiated-generic.rs b/src/test/codegen-units/item-collection/impl-in-non-instantiated-generic.rs index a3bfa67e1ae44..c43c254f33911 100644 --- a/src/test/codegen-units/item-collection/impl-in-non-instantiated-generic.rs +++ b/src/test/codegen-units/item-collection/impl-in-non-instantiated-generic.rs @@ -32,5 +32,3 @@ pub fn generic_function(x: T) -> (T, i32) { fn main() { 0i64.foo(); } - -//~ TRANS_ITEM drop-glue i8 diff --git a/src/test/codegen-units/item-collection/non-generic-closures.rs b/src/test/codegen-units/item-collection/non-generic-closures.rs index bf8804e12ce49..ba77266d07248 100644 --- a/src/test/codegen-units/item-collection/non-generic-closures.rs +++ b/src/test/codegen-units/item-collection/non-generic-closures.rs @@ -59,5 +59,3 @@ fn main() { fn run_closure(f: &Fn(i32)) { f(3); } - -//~ TRANS_ITEM drop-glue i8 diff --git a/src/test/codegen-units/item-collection/statics-and-consts.rs b/src/test/codegen-units/item-collection/statics-and-consts.rs index 7c8b2b117ef7c..89bc620b7c552 100644 --- a/src/test/codegen-units/item-collection/statics-and-consts.rs +++ b/src/test/codegen-units/item-collection/statics-and-consts.rs @@ -60,5 +60,3 @@ fn main() { //~ TRANS_ITEM static statics_and_consts::foo[0]::STATIC2[2] //~ TRANS_ITEM fn statics_and_consts::main[0] - -//~ TRANS_ITEM drop-glue i8 From 89edd96be86fd65d63f63a208062c8baf86e7d7c Mon Sep 17 00:00:00 2001 From: James Miller Date: Sat, 16 Apr 2016 19:45:28 +1200 Subject: [PATCH 4/9] Fix translation of `Assign`/`AssignOp` as rvalues In code like `let x = y = z;`, `y = z` goes through `as_rvalue`, which didn't handle it. Now it translates the assignment and produces `()` directly. --- src/librustc_mir/build/expr/as_rvalue.rs | 7 +++++-- src/librustc_mir/build/misc.rs | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/build/expr/as_rvalue.rs b/src/librustc_mir/build/expr/as_rvalue.rs index b340d933e64c3..8992381135ea8 100644 --- a/src/librustc_mir/build/expr/as_rvalue.rs +++ b/src/librustc_mir/build/expr/as_rvalue.rs @@ -189,6 +189,11 @@ impl<'a,'tcx> Builder<'a,'tcx> { block.and(Rvalue::Aggregate(AggregateKind::Adt(adt_def, variant_index, substs), fields)) } + ExprKind::Assign { .. } | + ExprKind::AssignOp { .. } => { + block = unpack!(this.stmt_expr(block, expr)); + block.and(this.unit_rvalue()) + } ExprKind::Literal { .. } | ExprKind::Block { .. } | ExprKind::Match { .. } | @@ -201,8 +206,6 @@ impl<'a,'tcx> Builder<'a,'tcx> { ExprKind::Index { .. } | ExprKind::VarRef { .. } | ExprKind::SelfRef | - ExprKind::Assign { .. } | - ExprKind::AssignOp { .. } | ExprKind::Break { .. } | ExprKind::Continue { .. } | ExprKind::Return { .. } | diff --git a/src/librustc_mir/build/misc.rs b/src/librustc_mir/build/misc.rs index 86f15a6319399..5daaf37d87814 100644 --- a/src/librustc_mir/build/misc.rs +++ b/src/librustc_mir/build/misc.rs @@ -46,6 +46,10 @@ impl<'a,'tcx> Builder<'a,'tcx> { Operand::Constant(constant) } + pub fn unit_rvalue(&mut self) -> Rvalue<'tcx> { + Rvalue::Aggregate(AggregateKind::Tuple, vec![]) + } + pub fn push_usize(&mut self, block: BasicBlock, scope_id: ScopeId, From c55d9e591b1f66d4677dac2efda6b70afcd4b345 Mon Sep 17 00:00:00 2001 From: James Miller Date: Sun, 17 Apr 2016 14:37:52 +1200 Subject: [PATCH 5/9] Move zero-sized type handling logic to `new_operand` `new_operand` now checks the type it's given and either creates the nil value itself, or produces an empty operand. --- src/librustc_trans/mir/mod.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index b7661ce039ca3..75d9ca32a21de 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -16,7 +16,7 @@ use rustc::mir::repr as mir; use rustc::mir::tcx::LvalueTy; use session::config::FullDebugInfo; use base; -use common::{self, Block, BlockAndBuilder, FunctionContext}; +use common::{self, Block, BlockAndBuilder, CrateContext, FunctionContext}; use debuginfo::{self, declare_local, DebugLoc, VariableAccess, VariableKind}; use machine; use type_of; @@ -109,12 +109,21 @@ enum TempRef<'tcx> { } impl<'tcx> TempRef<'tcx> { - fn new_operand(val: OperandValue, ty: ty::Ty<'tcx>) -> TempRef<'tcx> { - let op = OperandRef { - val: val, - ty: ty - }; - TempRef::Operand(Some(op)) + fn new_operand<'bcx>(ccx: &CrateContext<'bcx, 'tcx>, + ty: ty::Ty<'tcx>) -> TempRef<'tcx> { + if common::type_is_zero_size(ccx, ty) { + // Zero-size temporaries aren't always initialized, which + // doesn't matter because they don't contain data, but + // we need something in the operand. + let val = OperandValue::Immediate(common::C_nil(ccx)); + let op = OperandRef { + val: val, + ty: ty + }; + TempRef::Operand(Some(op)) + } else { + TempRef::Operand(None) + } } } @@ -160,17 +169,11 @@ pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) { TempRef::Lvalue(LvalueRef::alloca(&bcx, mty, &format!("temp{:?}", i))) - } else if common::type_is_zero_size(bcx.ccx(), mty) { - // Zero-size temporaries aren't always initialized, which - // doesn't matter because they don't contain data, but - // we need something in the operand. - let val = OperandValue::Immediate(common::C_nil(bcx.ccx())); - TempRef::new_operand(val, mty) } else { // If this is an immediate temp, we do not create an // alloca in advance. Instead we wait until we see the // definition and update the operand there. - TempRef::Operand(None) + TempRef::new_operand(bcx.ccx(), mty) }) .collect(); From 0e3b37a52e618f93f8b1357744d507b1b527a0fa Mon Sep 17 00:00:00 2001 From: James Miller Date: Sun, 17 Apr 2016 16:35:37 +1200 Subject: [PATCH 6/9] Fix codegen-units tests I'm not sure what the signficance of `drop-glue i8` is, nor why one of the tests had it appear while the others had it disappear. Either way it doesn't seem like the presence or absense of it is the focus of the tests. --- src/test/codegen-units/item-collection/statics-and-consts.rs | 2 ++ .../codegen-units/partitioning/inlining-from-extern-crate.rs | 2 -- src/test/codegen-units/partitioning/local-inlining.rs | 2 -- .../codegen-units/partitioning/local-transitive-inlining.rs | 2 -- 4 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/test/codegen-units/item-collection/statics-and-consts.rs b/src/test/codegen-units/item-collection/statics-and-consts.rs index 89bc620b7c552..7c8b2b117ef7c 100644 --- a/src/test/codegen-units/item-collection/statics-and-consts.rs +++ b/src/test/codegen-units/item-collection/statics-and-consts.rs @@ -60,3 +60,5 @@ fn main() { //~ TRANS_ITEM static statics_and_consts::foo[0]::STATIC2[2] //~ TRANS_ITEM fn statics_and_consts::main[0] + +//~ TRANS_ITEM drop-glue i8 diff --git a/src/test/codegen-units/partitioning/inlining-from-extern-crate.rs b/src/test/codegen-units/partitioning/inlining-from-extern-crate.rs index f4732a7bcf8fb..469f2c08c39c2 100644 --- a/src/test/codegen-units/partitioning/inlining-from-extern-crate.rs +++ b/src/test/codegen-units/partitioning/inlining-from-extern-crate.rs @@ -57,5 +57,3 @@ mod mod2 { cgu_explicit_inlining::never_inlined(); } } - -//~ TRANS_ITEM drop-glue i8 diff --git a/src/test/codegen-units/partitioning/local-inlining.rs b/src/test/codegen-units/partitioning/local-inlining.rs index dfd8f725b61dd..d2bfa83834665 100644 --- a/src/test/codegen-units/partitioning/local-inlining.rs +++ b/src/test/codegen-units/partitioning/local-inlining.rs @@ -50,5 +50,3 @@ mod non_user { } } - -//~ TRANS_ITEM drop-glue i8 diff --git a/src/test/codegen-units/partitioning/local-transitive-inlining.rs b/src/test/codegen-units/partitioning/local-transitive-inlining.rs index ea3a1cd34be32..2e47dc5c9020f 100644 --- a/src/test/codegen-units/partitioning/local-transitive-inlining.rs +++ b/src/test/codegen-units/partitioning/local-transitive-inlining.rs @@ -50,5 +50,3 @@ mod non_user { } } - -//~ TRANS_ITEM drop-glue i8 From 3bcee269b50def69d73bd588b1619ec6a4756662 Mon Sep 17 00:00:00 2001 From: James Miller Date: Wed, 20 Apr 2016 15:27:15 +1200 Subject: [PATCH 7/9] Handle immediate tuples in `trans_arguments_untupled` Use either getelementptr or extractvalue depending on whether or not the tuple is immediate or not. --- src/librustc_trans/mir/block.rs | 72 ++++++++++++++++----------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/src/librustc_trans/mir/block.rs b/src/librustc_trans/mir/block.rs index f70dc0183fdf1..d6a42bea22a6e 100644 --- a/src/librustc_trans/mir/block.rs +++ b/src/librustc_trans/mir/block.rs @@ -436,47 +436,47 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { fn_ty: &FnType, next_idx: &mut usize, callee: &mut CalleeData) { - // FIXME: consider having some optimization to avoid tupling/untupling - // (and storing/loading in the case of immediates) - - // avoid trans_operand for pointless copying - let lv = match *operand { - mir::Operand::Consume(ref lvalue) => self.trans_lvalue(bcx, lvalue), - mir::Operand::Constant(ref constant) => { - // FIXME: consider being less pessimized - if constant.ty.is_nil() { - return; - } - - let ty = bcx.monomorphize(&constant.ty); - let lv = LvalueRef::alloca(bcx, ty, "__untuple_alloca"); - let constant = self.trans_constant(bcx, constant); - self.store_operand(bcx, lv.llval, constant); - lv - } - }; + let tuple = self.trans_operand(bcx, operand); - let lv_ty = lv.ty.to_ty(bcx.tcx()); - let result_types = match lv_ty.sty { + let arg_types = match tuple.ty.sty { ty::TyTuple(ref tys) => tys, - _ => span_bug!( - self.mir.span, - "bad final argument to \"rust-call\" fn {:?}", lv_ty) + _ => span_bug!(self.mir.span, + "bad final argument to \"rust-call\" fn {:?}", tuple.ty) }; - let base_repr = adt::represent_type(bcx.ccx(), lv_ty); - let base = adt::MaybeSizedValue::sized(lv.llval); - for (n, &ty) in result_types.iter().enumerate() { - let ptr = adt::trans_field_ptr_builder(bcx, &base_repr, base, Disr(0), n); - let val = if common::type_is_fat_ptr(bcx.tcx(), ty) { - let (lldata, llextra) = load_fat_ptr(bcx, ptr); - FatPtr(lldata, llextra) - } else { - // Don't bother loading the value, trans_argument will. - Ref(ptr) - }; - self.trans_argument(bcx, val, llargs, fn_ty, next_idx, callee); + // Handle both by-ref and immediate tuples. This gives us the option of + match tuple.val { + Ref(llval) => { + let base_repr = adt::represent_type(bcx.ccx(), tuple.ty); + let base = adt::MaybeSizedValue::sized(llval); + for (n, &ty) in arg_types.iter().enumerate() { + let ptr = adt::trans_field_ptr_builder(bcx, &base_repr, base, Disr(0), n); + let val = if common::type_is_fat_ptr(bcx.tcx(), ty) { + let (lldata, llextra) = load_fat_ptr(bcx, ptr); + FatPtr(lldata, llextra) + } else { + // trans_argument will load this if it needs to + Ref(ptr) + }; + self.trans_argument(bcx, val, llargs, fn_ty, next_idx, callee); + } + + } + Immediate(llval) => { + for (n, &ty) in arg_types.iter().enumerate() { + let mut elem = bcx.extract_value(llval, n); + // Truncate bools to i1, if needed + if ty.is_bool() && common::val_ty(elem) != Type::i1(bcx.ccx()) { + elem = bcx.trunc(elem, Type::i1(bcx.ccx())); + } + // If the tuple is immediate, the elements are as well + let val = Immediate(elem); + self.trans_argument(bcx, val, llargs, fn_ty, next_idx, callee); + } + } + FatPtr(_, _) => bug!("tuple is a fat pointer?!") } + } fn get_personality_slot(&mut self, bcx: &BlockAndBuilder<'bcx, 'tcx>) -> ValueRef { From b5d7783546f938c7c2903b4d5143ff8e8e612674 Mon Sep 17 00:00:00 2001 From: James Miller Date: Wed, 20 Apr 2016 18:10:40 +1200 Subject: [PATCH 8/9] Check when building invoke as well as calls LLVM's assertion doesn't provide much insight as to what the problem was. We were already checking `call` instructions ourselves, so this brings the checks from there to `invoke`. Both the `invoke` and `call` checking is controlled by `debug_assertions`. --- src/librustc_trans/builder.rs | 56 +++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/src/librustc_trans/builder.rs b/src/librustc_trans/builder.rs index 92fb342497b5a..c7de7fd3695d1 100644 --- a/src/librustc_trans/builder.rs +++ b/src/librustc_trans/builder.rs @@ -165,8 +165,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { args: &[ValueRef], then: BasicBlockRef, catch: BasicBlockRef, - bundle: Option<&OperandBundleDef>) - -> ValueRef { + bundle: Option<&OperandBundleDef>) -> ValueRef { self.count_insn("invoke"); debug!("Invoke {:?} with args ({})", @@ -176,6 +175,31 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { .collect::>() .join(", ")); + if cfg!(debug_assertions) { + let mut fn_ty = val_ty(llfn); + // Strip off pointers + while fn_ty.kind() == llvm::TypeKind::Pointer { + fn_ty = fn_ty.element_type(); + } + + assert!(fn_ty.kind() == llvm::TypeKind::Function, + "builder::invoke not passed a function"); + + let param_tys = fn_ty.func_params(); + + let iter = param_tys.into_iter() + .zip(args.iter().map(|&v| val_ty(v))); + for (i, (expected_ty, actual_ty)) in iter.enumerate() { + if expected_ty != actual_ty { + bug!("Type mismatch in invoke of {:?}. \ + Expected {:?} for param {}, got {:?}", + Value(llfn), + expected_ty, i, actual_ty); + + } + } + } + let bundle = bundle.as_ref().map(|b| b.raw()).unwrap_or(0 as *mut _); unsafe { @@ -856,26 +880,28 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { .collect::>() .join(", ")); - let mut fn_ty = val_ty(llfn); - // Strip off pointers - while fn_ty.kind() == llvm::TypeKind::Pointer { - fn_ty = fn_ty.element_type(); - } + if cfg!(debug_assertions) { + let mut fn_ty = val_ty(llfn); + // Strip off pointers + while fn_ty.kind() == llvm::TypeKind::Pointer { + fn_ty = fn_ty.element_type(); + } - assert!(fn_ty.kind() == llvm::TypeKind::Function, - "builder::call not passed a function"); + assert!(fn_ty.kind() == llvm::TypeKind::Function, + "builder::call not passed a function"); - let param_tys = fn_ty.func_params(); + let param_tys = fn_ty.func_params(); - let iter = param_tys.into_iter() - .zip(args.iter().map(|&v| val_ty(v))); - for (i, (expected_ty, actual_ty)) in iter.enumerate() { - if expected_ty != actual_ty { - bug!("Type mismatch in function call of {:?}. \ + let iter = param_tys.into_iter() + .zip(args.iter().map(|&v| val_ty(v))); + for (i, (expected_ty, actual_ty)) in iter.enumerate() { + if expected_ty != actual_ty { + bug!("Type mismatch in function call of {:?}. \ Expected {:?} for param {}, got {:?}", Value(llfn), expected_ty, i, actual_ty); + } } } From 5bda576cd6b3be40f62a37e134ee7245e911fb8b Mon Sep 17 00:00:00 2001 From: James Miller Date: Thu, 21 Apr 2016 11:43:01 +1200 Subject: [PATCH 9/9] Factor out function call checking to a helper method The logic for checking `call` and `invoke` instructions was duplicated between them, so factor it out to a helper method. --- src/librustc_trans/builder.rs | 77 +++++++++++++-------------------- src/librustc_trans/mir/block.rs | 2 +- 2 files changed, 30 insertions(+), 49 deletions(-) diff --git a/src/librustc_trans/builder.rs b/src/librustc_trans/builder.rs index c7de7fd3695d1..9f032cdbfe513 100644 --- a/src/librustc_trans/builder.rs +++ b/src/librustc_trans/builder.rs @@ -175,30 +175,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { .collect::>() .join(", ")); - if cfg!(debug_assertions) { - let mut fn_ty = val_ty(llfn); - // Strip off pointers - while fn_ty.kind() == llvm::TypeKind::Pointer { - fn_ty = fn_ty.element_type(); - } - - assert!(fn_ty.kind() == llvm::TypeKind::Function, - "builder::invoke not passed a function"); - - let param_tys = fn_ty.func_params(); - - let iter = param_tys.into_iter() - .zip(args.iter().map(|&v| val_ty(v))); - for (i, (expected_ty, actual_ty)) in iter.enumerate() { - if expected_ty != actual_ty { - bug!("Type mismatch in invoke of {:?}. \ - Expected {:?} for param {}, got {:?}", - Value(llfn), - expected_ty, i, actual_ty); - - } - } - } + check_call("invoke", llfn, args); let bundle = bundle.as_ref().map(|b| b.raw()).unwrap_or(0 as *mut _); @@ -880,30 +857,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { .collect::>() .join(", ")); - if cfg!(debug_assertions) { - let mut fn_ty = val_ty(llfn); - // Strip off pointers - while fn_ty.kind() == llvm::TypeKind::Pointer { - fn_ty = fn_ty.element_type(); - } - - assert!(fn_ty.kind() == llvm::TypeKind::Function, - "builder::call not passed a function"); - - let param_tys = fn_ty.func_params(); - - let iter = param_tys.into_iter() - .zip(args.iter().map(|&v| val_ty(v))); - for (i, (expected_ty, actual_ty)) in iter.enumerate() { - if expected_ty != actual_ty { - bug!("Type mismatch in function call of {:?}. \ - Expected {:?} for param {}, got {:?}", - Value(llfn), - expected_ty, i, actual_ty); - - } - } - } + check_call("call", llfn, args); let bundle = bundle.as_ref().map(|b| b.raw()).unwrap_or(0 as *mut _); @@ -1147,3 +1101,30 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } } + +fn check_call(typ: &str, llfn: ValueRef, args: &[ValueRef]) { + if cfg!(debug_assertions) { + let mut fn_ty = val_ty(llfn); + // Strip off pointers + while fn_ty.kind() == llvm::TypeKind::Pointer { + fn_ty = fn_ty.element_type(); + } + + assert!(fn_ty.kind() == llvm::TypeKind::Function, + "builder::{} not passed a function", typ); + + let param_tys = fn_ty.func_params(); + + let iter = param_tys.into_iter() + .zip(args.iter().map(|&v| val_ty(v))); + for (i, (expected_ty, actual_ty)) in iter.enumerate() { + if expected_ty != actual_ty { + bug!("Type mismatch in function call of {:?}. \ + Expected {:?} for param {}, got {:?}", + Value(llfn), + expected_ty, i, actual_ty); + + } + } + } +} diff --git a/src/librustc_trans/mir/block.rs b/src/librustc_trans/mir/block.rs index d6a42bea22a6e..d0b47934bcf16 100644 --- a/src/librustc_trans/mir/block.rs +++ b/src/librustc_trans/mir/block.rs @@ -444,7 +444,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { "bad final argument to \"rust-call\" fn {:?}", tuple.ty) }; - // Handle both by-ref and immediate tuples. This gives us the option of + // Handle both by-ref and immediate tuples. match tuple.val { Ref(llval) => { let base_repr = adt::represent_type(bcx.ccx(), tuple.ty);