]> git.lizzy.rs Git - rust.git/commitdiff
Various improvements to MIR and LLVM IR Construction
authorJames Miller <james@aatch.net>
Fri, 15 Apr 2016 00:36:16 +0000 (12:36 +1200)
committerJames Miller <james@aatch.net>
Thu, 28 Apr 2016 01:17:43 +0000 (13:17 +1200)
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
src/librustc_mir/build/expr/into.rs
src/librustc_mir/build/expr/mod.rs
src/librustc_mir/build/scope.rs
src/librustc_trans/mir/analyze.rs
src/librustc_trans/mir/mod.rs
src/librustc_trans/mir/rvalue.rs
src/librustc_trans/mir/statement.rs

index 8c98408e2390ae2cfb8998c4720681a765824396..fc67bc7ded0837ea8953bb709b748649eb4e027a 100644 (file)
@@ -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 @@ pub fn ast_block(&mut self,
                     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 @@ pub fn ast_block(&mut self,
             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<F>(&mut self,
+                            span: Span,
+                            label: Option<CodeExtent>,
+                            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()
+    }
 }
index fe32f1de0c52053306ee6286d2b188a1432f7adc..b7729b01737bad6628f5fbd22c76039aa603c92d 100644 (file)
 
 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 @@ pub fn into_expr(&mut self,
                 }
                 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 @@ pub fn into_expr(&mut self,
                 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 @@ pub fn into_expr(&mut self,
             }
         }
     }
-
-    fn break_or_continue<F>(&mut self,
-                            span: Span,
-                            label: Option<CodeExtent>,
-                            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()
-    }
 }
index 0f168f307aa01b5cb07c1f7a922ed0fea5708057..b131b0ad5c11d593eb303b9ba84767c87a5411d6 100644 (file)
@@ -75,5 +75,5 @@
 mod as_rvalue;
 mod as_operand;
 mod as_temp;
-mod category;
+pub mod category;
 mod into;
index bfbf27d46d005f5a7aa72b984435c203065e06ee..95c931df9e5cc3a7f56cd5c01faade649b59f762 100644 (file)
@@ -497,8 +497,11 @@ pub fn diverge_cleanup(&mut self) -> Option<BasicBlock> {
     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();
index f721e88a954139138cc03e3bb9a84950dfdad1db..6730f8c0fcc79a9163c10b8874e48068bddb53ae 100644 (file)
@@ -20,7 +20,7 @@
 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 @@ fn mark_assigned(&mut self, temp: usize) {
     }
 }
 
-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 @@ fn visit_assign(&mut self,
         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);
                 }
             }
index 7ec3c9345be58fd16bed4092c8bc46d4e257e95f..a5e116ae7b6262f8e5dabd88ea0397a88bb354ac 100644 (file)
@@ -34,7 +34,7 @@
 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
index 641603f5aaad9a88538ca7662f18df7e348d93ee..3050d2c5290b784e7e34f025936c6acf7e362c59 100644 (file)
@@ -18,7 +18,7 @@
 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 tvec;
 use value::Value;
 use Disr;
+use glue;
 
 use super::MirContext;
 use super::operand::{OperandRef, OperandValue};
@@ -217,7 +218,9 @@ pub fn trans_rvalue(&mut self,
             }
 
             _ => {
-                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 @@ pub fn trans_rvalue_operand(&mut self,
                                 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 @@ pub fn trans_rvalue_operand(&mut self,
                 (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 @@ pub fn trans_scalar_binop(&mut self,
     }
 }
 
-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.
 }
index e4967cead07e9b4098f5203bd33bceff0ee4c327..6ec4ee3ce9326ae7e8da3a39ba0f8c54cb8ea936 100644 (file)
@@ -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 @@ pub fn trans_statement(&mut self,
                                 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
+                                }
                             }
                         }
                     }