Auto merge of #30044 - nikomatsakis:issue-29466, r=arielb1

The graph extent mechanism is not good. I have some ideas for a better replacement, but this PR simply removes it. It also stops recursing on statement scopes and processes them using an "on the heap" stack, which fixes #29466.

r? @dotdash

Author: bors

src/librustc_mir/build/cfg.rs

@@ -26,13 +26,6 @@ impl<'tcx> CFG<'tcx> {
         &mut self.basic_blocks[blk.index()]
     }
 
-    pub fn end_point(&self, block: BasicBlock) -> ExecutionPoint {
-        ExecutionPoint {
-            block: block,
-            statement: self.block_data(block).statements.len() as u32,
-        }
-    }
-
     pub fn start_new_block(&mut self) -> BasicBlock {
         let node_index = self.basic_blocks.len();
         self.basic_blocks.push(BasicBlockData::new(Terminator::Diverge));

src/librustc_mir/build/expr/as_rvalue.rs

@@ -70,7 +70,7 @@ impl<'a,'tcx> Builder<'a,'tcx> {
                 this.cfg.push_assign(block, expr_span, &result, rvalue);
 
                 // schedule a shallow free of that memory, lest we unwind:
-                let extent = this.extent_of_innermost_scope().unwrap();
+                let extent = this.extent_of_innermost_scope();
                 this.schedule_drop(expr_span, extent, DropKind::Free, &result, value_ty);
 
                 // initialize the box contents:

src/librustc_mir/build/expr/into.rs

@@ -206,7 +206,7 @@ impl<'a,'tcx> Builder<'a,'tcx> {
             }
             ExprKind::Return { value } => {
                 unpack!(block = this.into(&Lvalue::ReturnPointer, block, value));
-                let extent = this.extent_of_outermost_scope().unwrap();
+                let extent = this.extent_of_outermost_scope();
                 this.exit_scope(expr_span, extent, block, END_BLOCK);
                 this.cfg.start_new_block().unit()
             }

src/librustc_mir/build/matches/mod.rs

@@ -42,7 +42,7 @@ impl<'a,'tcx> Builder<'a,'tcx> {
         // suitable extent for all of the bindings in this match. It's
         // easiest to do this up front because some of these arms may
         // be unreachable or reachable multiple times.
-        let var_extent = self.extent_of_innermost_scope().unwrap();
+        let var_extent = self.extent_of_innermost_scope();
         for arm in &arms {
             self.declare_bindings(var_extent, &arm.patterns[0]);
         }

src/librustc_mir/build/mod.rs

@@ -19,7 +19,6 @@ use syntax::codemap::Span;
 
 struct Builder<'a, 'tcx: 'a> {
     hir: Cx<'a, 'tcx>,
-    extents: FnvHashMap<CodeExtent, Vec<GraphExtent>>,
     cfg: CFG<'tcx>,
     scopes: Vec<scope::Scope<'tcx>>,
     loop_scopes: Vec<scope::LoopScope>,
@@ -92,7 +91,6 @@ pub fn construct<'a,'tcx>(mut hir: Cx<'a,'tcx>,
     let mut builder = Builder {
         hir: hir,
         cfg: cfg,
-        extents: FnvHashMap(),
         scopes: vec![],
         loop_scopes: vec![],
         temp_decls: temp_decls,
@@ -117,7 +115,6 @@ pub fn construct<'a,'tcx>(mut hir: Cx<'a,'tcx>,
 
     Mir {
         basic_blocks: builder.cfg.basic_blocks,
-        extents: builder.extents,
         var_decls: builder.var_decls,
         arg_decls: arg_decls,
         temp_decls: builder.temp_decls,

src/librustc_mir/build/scope.rs

@@ -94,7 +94,6 @@ use syntax::codemap::Span;
 
 pub struct Scope<'tcx> {
     extent: CodeExtent,
-    exits: Vec<ExecutionPoint>,
     drops: Vec<(DropKind, Span, Lvalue<'tcx>)>,
     cached_block: Option<BasicBlock>,
 }
@@ -116,7 +115,7 @@ impl<'a,'tcx> Builder<'a,'tcx> {
                                -> BlockAnd<R>
         where F: FnOnce(&mut Builder<'a, 'tcx>) -> BlockAnd<R>
     {
-        let extent = self.extent_of_innermost_scope().unwrap();
+        let extent = self.extent_of_innermost_scope();
         let loop_scope = LoopScope {
             extent: extent.clone(),
             continue_block: loop_block,
@@ -128,60 +127,51 @@ impl<'a,'tcx> Builder<'a,'tcx> {
         r
     }
 
-    /// Start a scope. The closure `f` should translate the contents
-    /// of the scope. See module comment for more details.
-    pub fn in_scope<F, R>(&mut self, extent: CodeExtent, block: BasicBlock, f: F) -> BlockAnd<R>
+    /// Convenience wrapper that pushes a scope and then executes `f`
+    /// to build its contents, popping the scope afterwards.
+    pub fn in_scope<F, R>(&mut self, extent: CodeExtent, mut block: BasicBlock, f: F) -> BlockAnd<R>
         where F: FnOnce(&mut Builder<'a, 'tcx>) -> BlockAnd<R>
     {
         debug!("in_scope(extent={:?}, block={:?})", extent, block);
+        self.push_scope(extent, block);
+        let rv = unpack!(block = f(self));
+        self.pop_scope(extent, block);
+        debug!("in_scope: exiting extent={:?} block={:?}", extent, block);
+        block.and(rv)
+    }
 
-        let start_point = self.cfg.end_point(block);
+    /// Push a scope onto the stack. You can then build code in this
+    /// scope and call `pop_scope` afterwards. Note that these two
+    /// calls must be paired; using `in_scope` as a convenience
+    /// wrapper maybe preferable.
+    pub fn push_scope(&mut self, extent: CodeExtent, block: BasicBlock) {
+        debug!("push_scope({:?}, {:?})", extent, block);
 
         // push scope, execute `f`, then pop scope again
         self.scopes.push(Scope {
             extent: extent.clone(),
             drops: vec![],
-            exits: vec![],
             cached_block: None,
         });
-        let BlockAnd(fallthrough_block, rv) = f(self);
-        let mut scope = self.scopes.pop().unwrap();
+    }
+
+    /// Pops a scope, which should have extent `extent`, adding any
+    /// drops onto the end of `block` that are needed.  This must
+    /// match 1-to-1 with `push_scope`.
+    pub fn pop_scope(&mut self, extent: CodeExtent, block: BasicBlock) {
+        debug!("pop_scope({:?}, {:?})", extent, block);
+        let scope = self.scopes.pop().unwrap();
+
+        assert_eq!(scope.extent, extent);
 
         // add in any drops needed on the fallthrough path (any other
         // exiting paths, such as those that arise from `break`, will
         // have drops already)
         for (kind, span, lvalue) in scope.drops {
-            self.cfg.push_drop(fallthrough_block, span, kind, &lvalue);
+            self.cfg.push_drop(block, span, kind, &lvalue);
         }
-
-        // add the implicit fallthrough edge
-        scope.exits.push(self.cfg.end_point(fallthrough_block));
-
-        // compute the extent from start to finish and store it in the graph
-        let graph_extent = self.graph_extent(start_point, scope.exits);
-        self.extents.entry(extent)
-                    .or_insert(vec![])
-                    .push(graph_extent);
-
-        debug!("in_scope: exiting extent={:?} fallthrough_block={:?}", extent, fallthrough_block);
-        fallthrough_block.and(rv)
     }
 
-    /// Creates a graph extent (SEME region) from an entry point and
-    /// exit points.
-    fn graph_extent(&self, entry: ExecutionPoint, exits: Vec<ExecutionPoint>) -> GraphExtent {
-        if exits.len() == 1 && entry.block == exits[0].block {
-            GraphExtent {
-                entry: entry,
-                exit: GraphExtentExit::Statement(exits[0].statement),
-            }
-        } else {
-            GraphExtent {
-                entry: entry,
-                exit: GraphExtentExit::Points(exits),
-            }
-        }
-    }
 
     /// Finds the loop scope for a given label. This is used for
     /// resolving `break` and `continue`.
@@ -232,8 +222,6 @@ impl<'a,'tcx> Builder<'a,'tcx> {
             for &(kind, drop_span, ref lvalue) in &scope.drops {
                 self.cfg.push_drop(block, drop_span, kind, lvalue);
             }
-
-            scope.exits.push(self.cfg.end_point(block));
         }
 
         self.cfg.terminate(block, Terminator::Goto { target: target });
@@ -272,12 +260,12 @@ impl<'a,'tcx> Builder<'a,'tcx> {
         }
     }
 
-    pub fn extent_of_innermost_scope(&self) -> Option<CodeExtent> {
-        self.scopes.last().map(|scope| scope.extent)
+    pub fn extent_of_innermost_scope(&self) -> CodeExtent {
+        self.scopes.last().map(|scope| scope.extent).unwrap()
     }
 
-    pub fn extent_of_outermost_scope(&self) -> Option<CodeExtent> {
-        self.scopes.first().map(|scope| scope.extent)
+    pub fn extent_of_outermost_scope(&self) -> CodeExtent {
+        self.scopes.first().map(|scope| scope.extent).unwrap()
     }
 }
 

src/librustc_mir/build/stmt.rs

@@ -14,48 +14,70 @@ use repr::*;
 
 impl<'a,'tcx> Builder<'a,'tcx> {
     pub fn stmts(&mut self, mut block: BasicBlock, stmts: Vec<StmtRef<'tcx>>) -> BlockAnd<()> {
-        for stmt in stmts {
-            unpack!(block = self.stmt(block, stmt));
-        }
-        block.unit()
-    }
-
-    pub fn stmt(&mut self, mut block: BasicBlock, stmt: StmtRef<'tcx>) -> BlockAnd<()> {
+        // This convoluted structure is to avoid using recursion as we walk down a list
+        // of statements. Basically, the structure we get back is something like:
+        //
+        //    let x = <init> in {
+        //       let y = <init> in {
+        //           expr1;
+        //           expr2;
+        //       }
+        //    }
+        //
+        // To process this, we keep a stack of (Option<CodeExtent>,
+        // vec::IntoIter<Stmt>) pairs.  At each point we pull off the
+        // top most pair and extract one statement from the
+        // iterator. Once it's complete, we pop the scope from the
+        // first half the pair.
         let this = self;
-        let Stmt { span, kind } = this.hir.mirror(stmt);
-        match kind {
-            StmtKind::Let { remainder_scope,
-                            init_scope,
-                            pattern,
-                            initializer: Some(initializer),
-                            stmts } => {
-                this.in_scope(remainder_scope, block, |this| {
-                    unpack!(block = this.in_scope(init_scope, block, |this| {
-                        this.expr_into_pattern(block, remainder_scope, pattern, initializer)
+        let mut stmt_lists = vec![(None, stmts.into_iter())];
+        while !stmt_lists.is_empty() {
+            let stmt = {
+                let &mut (_, ref mut stmts) = stmt_lists.last_mut().unwrap();
+                stmts.next()
+            };
+
+            let stmt = match stmt {
+                Some(stmt) => stmt,
+                None => {
+                    let (extent, _) = stmt_lists.pop().unwrap();
+                    if let Some(extent) = extent {
+                        this.pop_scope(extent, block);
+                    }
+                    continue
+                }
+            };
+
+            let Stmt { span, kind } = this.hir.mirror(stmt);
+            match kind {
+                StmtKind::Let { remainder_scope, init_scope, pattern, initializer, stmts } => {
+                    this.push_scope(remainder_scope, block);
+                    stmt_lists.push((Some(remainder_scope), stmts.into_iter()));
+                    unpack!(block = this.in_scope(init_scope, block, move |this| {
+                        // FIXME #30046                              ^~~~
+                        match initializer {
+                            Some(initializer) => {
+                                this.expr_into_pattern(block, remainder_scope, pattern, initializer)
+                            }
+                            None => {
+                                this.declare_bindings(remainder_scope, &pattern);
+                                block.unit()
+                            }
+                        }
                     }));
-                    this.stmts(block, stmts)
-                })
-            }
+                }
 
-            StmtKind::Let { remainder_scope, init_scope, pattern, initializer: None, stmts } => {
-                this.in_scope(remainder_scope, block, |this| {
-                    unpack!(block = this.in_scope(init_scope, block, |this| {
-                        this.declare_bindings(remainder_scope, &pattern);
+                StmtKind::Expr { scope, expr } => {
+                    unpack!(block = this.in_scope(scope, block, |this| {
+                        let expr = this.hir.mirror(expr);
+                        let temp = this.temp(expr.ty.clone());
+                        unpack!(block = this.into(&temp, block, expr));
+                        this.cfg.push_drop(block, span, DropKind::Deep, &temp);
                         block.unit()
                     }));
-                    this.stmts(block, stmts)
-                })
-            }
-
-            StmtKind::Expr { scope, expr } => {
-                this.in_scope(scope, block, |this| {
-                    let expr = this.hir.mirror(expr);
-                    let temp = this.temp(expr.ty.clone());
-                    unpack!(block = this.into(&temp, block, expr));
-                    this.cfg.push_drop(block, span, DropKind::Deep, &temp);
-                    block.unit()
-                })
+                }
             }
         }
+        block.unit()
     }
 }

src/librustc_mir/repr.rs

@@ -10,11 +10,9 @@
 
 use rustc::middle::const_eval::ConstVal;
 use rustc::middle::def_id::DefId;
-use rustc::middle::region::CodeExtent;
 use rustc::middle::subst::Substs;
 use rustc::middle::ty::{AdtDef, ClosureSubsts, FnOutput, Region, Ty};
 use rustc_back::slice;
-use rustc_data_structures::fnv::FnvHashMap;
 use rustc_front::hir::InlineAsm;
 use syntax::ast::Name;
 use syntax::codemap::Span;
@@ -23,15 +21,24 @@ use std::u32;
 
 /// Lowered representation of a single function.
 pub struct Mir<'tcx> {
+    /// List of basic blocks. References to basic block use a newtyped index type `BasicBlock`
+    /// that indexes into this vector.
     pub basic_blocks: Vec<BasicBlockData<'tcx>>,
 
+    /// Return type of the function.
     pub return_ty: FnOutput<'tcx>,
 
-    // for every node id
-    pub extents: FnvHashMap<CodeExtent, Vec<GraphExtent>>,
-
+    /// Variables: these are stack slots corresponding to user variables. They may be
+    /// assigned many times.
     pub var_decls: Vec<VarDecl<'tcx>>,
+
+    /// Args: these are stack slots corresponding to the input arguments.
     pub arg_decls: Vec<ArgDecl<'tcx>>,
+
+    /// Temp declarations: stack slots that for temporaries created by
+    /// the compiler. These are assigned once, but they are not SSA
+    /// values in that it is possible to borrow them and mutate them
+    /// through the resulting reference.
     pub temp_decls: Vec<TempDecl<'tcx>>,
 }
 
@@ -147,48 +154,6 @@ pub struct ArgDecl<'tcx> {
     pub ty: Ty<'tcx>,
 }
 
-///////////////////////////////////////////////////////////////////////////
-// Graph extents
-
-/// A moment in the flow of execution. It corresponds to a point in
-/// between two statements:
-///
-///    BB[block]:
-///                          <--- if statement == 0
-///        STMT[0]
-///                          <--- if statement == 1
-///        STMT[1]
-///        ...
-///                          <--- if statement == n-1
-///        STMT[n-1]
-///                          <--- if statement == n
-///
-/// where the block has `n` statements.
-#[derive(Copy, Clone, Debug, PartialEq, Eq)]
-pub struct ExecutionPoint {
-    pub block: BasicBlock,
-    pub statement: u32,
-}
-
-/// A single-entry-multiple-exit region in the graph. We build one of
-/// these for every node-id during MIR construction. By construction
-/// we are assured that the entry dominates all points within, and
-/// that, for every interior point X, it is postdominated by some exit.
-pub struct GraphExtent {
-    pub entry: ExecutionPoint,
-    pub exit: GraphExtentExit,
-}
-
-pub enum GraphExtentExit {
-    /// `Statement(X)`: a very common special case covering a span
-    /// that is local to a single block. It starts at the entry point
-    /// and extends until the start of statement `X` (non-inclusive).
-    Statement(u32),
-
-    /// The more general case where the exits are a set of points.
-    Points(Vec<ExecutionPoint>),
-}
-
 ///////////////////////////////////////////////////////////////////////////
 // BasicBlock