Stop invalidating predecessors cache when accessing unique basic block, invalidate cache when accessing unique terminator

Nashenas88 · Nashenas88 · commit f534d9f8c4bc · 2019-12-02T08:30:30.000-05:00
diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs
@@ -203,17 +203,38 @@ impl<'tcx> Body<'tcx> {
 
     #[inline]
     pub fn basic_blocks_mut(&mut self) -> &mut IndexVec<BasicBlock, BasicBlockData<'tcx>> {
-        self.predecessors_cache = None;
-//        self.cache.invalidate();
         &mut self.basic_blocks
     }
 
+    pub fn basic_block_terminator_opt_mut(&mut self, bb: BasicBlock) -> &mut Option<Terminator<'tcx>> {
+        self.predecessors_cache = None;
+        &mut self.basic_blocks[bb].terminator
+    }
+
+    pub fn basic_block_terminator_mut(&mut self, bb: BasicBlock) -> &mut Terminator<'tcx> {
+        self.predecessors_cache = None;
+/*
+        let data = &mut self.basic_blocks[bb];
+        if let Some(cache) = self.predecessors_cache.as_mut() {
+            for successor in data.terminator().successors() {
+                let successor_vec = &mut cache[*successor];
+                for i in (0..successor_vec.len()).rev() {
+                    if successor_vec[i] == bb {
+                        successor_vec.swap_remove(i);
+                        break;
+                    }
+                }
+            }
+        }
+*/
+
+        self.basic_blocks[bb].terminator_mut()
+    }
+
     #[inline]
     pub fn basic_blocks_and_local_decls_mut(
         &mut self,
     ) -> (&mut IndexVec<BasicBlock, BasicBlockData<'tcx>>, &mut LocalDecls<'tcx>) {
-        self.predecessors_cache = None;
-//        self.cache.invalidate();
         (&mut self.basic_blocks, &mut self.local_decls)
     }
 
@@ -1358,6 +1379,10 @@ impl<'tcx> BasicBlockData<'tcx> {
         BasicBlockData { statements: vec![], terminator, is_cleanup: false }
     }
 
+    pub fn terminator_opt(&self) -> &Option<Terminator<'tcx>> {
+        &self.terminator
+    }
+
     /// Accessor for terminator.
     ///
     /// Terminator may not be None after construction of the basic block is complete. This accessor
@@ -1366,10 +1391,17 @@ impl<'tcx> BasicBlockData<'tcx> {
         self.terminator.as_ref().expect("invalid terminator state")
     }
 
-    pub fn terminator_mut(&mut self) -> &mut Terminator<'tcx> {
+    // This cannot be public since changing the terminator will break the predecessors cache in Body
+    // To do so outside of this module, use Body::basic_block_terminator_mut(BasicBlock)
+    fn terminator_mut(&mut self) -> &mut Terminator<'tcx> {
         self.terminator.as_mut().expect("invalid terminator state")
     }
 
+    // This can be public since changing the kind will not break the predecessors cache in Body
+    pub fn terminator_kind_mut(&mut self) -> &mut TerminatorKind<'tcx> {
+        &mut self.terminator_mut().kind
+    }
+
     pub fn retain_statements<F>(&mut self, mut f: F)
     where
         F: FnMut(&mut Statement<'_>) -> bool,
diff --git a/src/librustc/mir/traversal.rs b/src/librustc/mir/traversal.rs
@@ -55,7 +55,7 @@ impl<'a, 'tcx> Iterator for Preorder<'a, 'tcx> {
 
             let data = &self.body[idx];
 
-            if let Some(ref term) = data.terminator {
+            if let Some(ref term) = data.terminator_opt() {
                 self.worklist.extend(term.successors());
             }
 
@@ -117,7 +117,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> {
 
         let data = &po.body[root];
 
-        if let Some(ref term) = data.terminator {
+        if let Some(ref term) = data.terminator_opt() {
             po.visited.insert(root);
             po.visit_stack.push((root, term.successors()));
             po.traverse_successor();
@@ -186,7 +186,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> {
             };
 
             if self.visited.insert(bb) {
-                if let Some(term) = &self.body[bb].terminator {
+                if let Some(term) = &self.body[bb].terminator_opt() {
                     self.visit_stack.push((bb, term.successors()));
                 }
             }
diff --git a/src/librustc/mir/visit.rs b/src/librustc/mir/visit.rs
@@ -793,6 +793,7 @@ macro_rules! make_mir_visitor {
             fn visit_location(&mut self, body: & $($mutability)? Body<'tcx>, location: Location) {
                 let basic_block = & $($mutability)? body[location.block];
                 if basic_block.statements.len() == location.statement_index {
+                    // TODO(nashenas88) how to ensure we clear the cache only in the mutable case...
                     if let Some(ref $($mutability)? terminator) = basic_block.terminator {
                         self.visit_terminator(terminator, location)
                     }
diff --git a/src/librustc_codegen_ssa/mir/mod.rs b/src/librustc_codegen_ssa/mir/mod.rs
@@ -265,7 +265,7 @@ fn create_funclets<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
 
         let funclet;
         let ret_llbb;
-        match mir[bb].terminator.as_ref().map(|t| &t.kind) {
+        match mir[bb].terminator_opt().as_ref().map(|t| &t.kind) {
             // This is a basic block that we're aborting the program for,
             // notably in an `extern` function. These basic blocks are inserted
             // so that we assert that `extern` functions do indeed not panic,
diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs
@@ -497,7 +497,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             ..
                         },
                         ..
-                    }) = bbd.terminator {
+                    }) = bbd.terminator_opt() {
                         if let Some(source)
                             = BorrowedContentSource::from_call(func.ty(self.body, tcx), tcx)
                         {
diff --git a/src/librustc_mir/build/cfg.rs b/src/librustc_mir/build/cfg.rs
@@ -64,7 +64,7 @@ impl<'tcx> CFG<'tcx> {
                      source_info: SourceInfo,
                      kind: TerminatorKind<'tcx>) {
         debug!("terminating block {:?} <- {:?}", block, kind);
-        debug_assert!(self.block_data(block).terminator.is_none(),
+        debug_assert!(self.block_data(block).terminator_opt().is_none(),
                       "terminate: block {:?}={:?} already has a terminator set",
                       block,
                       self.block_data(block));
diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs
@@ -731,7 +731,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
     fn finish(self) -> Body<'tcx> {
         for (index, block) in self.cfg.basic_blocks.iter().enumerate() {
-            if block.terminator.is_none() {
+            if block.terminator_opt().is_none() {
                 span_bug!(self.fn_span, "no terminator on block {:?}", index);
             }
         }
diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs
@@ -98,7 +98,7 @@ fn precompute_borrows_out_of_scope<'tcx>(
             // Add successor BBs to the work list, if necessary.
             let bb_data = &body[bb];
             assert!(hi == bb_data.statements.len());
-            for &succ_bb in bb_data.terminator.as_ref().unwrap().successors() {
+            for &succ_bb in bb_data.terminator().successors() {
                 visited.entry(succ_bb)
                     .and_modify(|lo| {
                         // `succ_bb` has been seen before. If it wasn't
diff --git a/src/librustc_mir/lints.rs b/src/librustc_mir/lints.rs
@@ -79,7 +79,7 @@ fn check_fn_for_unconditional_recursion(
 
         let block = &basic_blocks[bb];
 
-        if let Some(ref terminator) = block.terminator {
+        if let Some(ref terminator) = block.terminator_opt() {
             match terminator.kind {
                 TerminatorKind::Call { ref func, .. } => {
                     let func_ty = func.ty(body, tcx);
diff --git a/src/librustc_mir/transform/add_call_guards.rs b/src/librustc_mir/transform/add_call_guards.rs
@@ -46,8 +46,9 @@ impl AddCallGuards {
 
         let cur_len = body.basic_blocks().len();
 
-        for block in body.basic_blocks_mut() {
-            match block.terminator {
+        for bb in body.basic_blocks().indices() {
+            let is_cleanup = body.basic_blocks()[bb].is_cleanup;
+            match body.basic_block_terminator_opt_mut(bb) {
                 Some(Terminator {
                     kind: TerminatorKind::Call {
                         destination: Some((_, ref mut destination)),
@@ -60,9 +61,9 @@ impl AddCallGuards {
                     // It's a critical edge, break it
                     let call_guard = BasicBlockData {
                         statements: vec![],
-                        is_cleanup: block.is_cleanup,
+                        is_cleanup: is_cleanup,
                         terminator: Some(Terminator {
-                            source_info,
+                            source_info: *source_info,
                             kind: TerminatorKind::Goto { target: *destination }
                         })
                     };
diff --git a/src/librustc_mir/transform/generator.rs b/src/librustc_mir/transform/generator.rs
@@ -368,7 +368,7 @@ impl MutVisitor<'tcx> for TransformVisitor<'tcx> {
                 VariantIdx::new(RETURNED) // state for returned
             };
             data.statements.push(self.set_discr(state, source_info));
-            data.terminator.as_mut().unwrap().kind = TerminatorKind::Return;
+            *data.terminator_kind_mut() = TerminatorKind::Return;
         }
 
         self.super_basic_block_data(block, data);
@@ -852,10 +852,10 @@ fn insert_switch<'tcx>(
         is_cleanup: false,
     });
 
-    let blocks = body.basic_blocks_mut().iter_mut();
-
-    for target in blocks.flat_map(|b| b.terminator_mut().successors_mut()) {
-        *target = BasicBlock::new(target.index() + 1);
+    for bb in body.basic_blocks_mut().indices() {
+        for target in body.basic_block_terminator_mut(bb).successors_mut() {
+            *target = BasicBlock::new(target.index() + 1);
+        }
     }
 }
 
@@ -941,7 +941,7 @@ fn create_generator_drop_shim<'tcx>(
     insert_switch(&mut body, cases, &transform, TerminatorKind::Return);
 
     for block in body.basic_blocks_mut() {
-        let kind = &mut block.terminator_mut().kind;
+        let kind = block.terminator_kind_mut();
         if let TerminatorKind::GeneratorDrop = *kind {
             *kind = TerminatorKind::Return;
         }
diff --git a/src/librustc_mir/transform/inline.rs b/src/librustc_mir/transform/inline.rs
@@ -379,7 +379,7 @@ impl Inliner<'tcx> {
                    callsite: CallSite<'tcx>,
                    caller_body: &mut Body<'tcx>,
                    mut callee_body: Body<'tcx>) -> bool {
-        let terminator = caller_body[callsite.bb].terminator.take().unwrap();
+        let terminator = caller_body.basic_block_terminator_opt_mut(callsite.bb).take().unwrap();
         match terminator.kind {
             // FIXME: Handle inlining of diverging calls
             TerminatorKind::Call { args, destination: Some(destination), cleanup, .. } => {
@@ -496,12 +496,12 @@ impl Inliner<'tcx> {
                     kind: TerminatorKind::Goto { target: BasicBlock::new(bb_len) }
                 };
 
-                caller_body[callsite.bb].terminator = Some(terminator);
+                *caller_body.basic_block_terminator_opt_mut(callsite.bb) = Some(terminator);
 
                 true
             }
             kind => {
-                caller_body[callsite.bb].terminator = Some(Terminator {
+                *caller_body.basic_block_terminator_opt_mut(callsite.bb) = Some(Terminator {
                     source_info: terminator.source_info,
                     kind,
                 });
diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs
@@ -866,7 +866,7 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> {
             let terminator = if self.keep_original {
                 self.source[loc.block].terminator().clone()
             } else {
-                let terminator = self.source[loc.block].terminator_mut();
+                let terminator = self.source.basic_block_terminator_mut(loc.block);
                 let target = match terminator.kind {
                     TerminatorKind::Call { destination: Some((_, target)), .. } => target,
                     ref kind => {
@@ -891,7 +891,7 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> {
                     let last = self.promoted.basic_blocks().last().unwrap();
                     let new_target = self.new_block();
 
-                    *self.promoted[last].terminator_mut() = Terminator {
+                    *self.promoted.basic_block_terminator_mut(last) = Terminator {
                         kind: TerminatorKind::Call {
                             func,
                             args,
@@ -976,11 +976,12 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> {
                     }
                 },
                 Candidate::Argument { bb, index } => {
-                    let terminator = blocks[bb].terminator_mut();
-                    match terminator.kind {
+                    let data = &mut blocks[bb];
+                    let terminator_span = data.terminator().source_info.span;
+                    match data.terminator_kind_mut() {
                         TerminatorKind::Call { ref mut args, .. } => {
                             let ty = args[index].ty(local_decls, self.tcx);
-                            let span = terminator.source_info.span;
+                            let span = terminator_span;
                             let operand = Operand::Copy(promoted_place(ty, span));
                             mem::replace(&mut args[index], operand)
                         }
@@ -1103,8 +1104,8 @@ pub fn promote_candidates<'tcx>(
 
     // Eliminate assignments to, and drops of promoted temps.
     let promoted = |index: Local| temps[index] == TempState::PromotedOut;
-    for block in body.basic_blocks_mut() {
-        block.statements.retain(|statement| {
+    for bb in body.basic_blocks().indices() {
+        body.basic_blocks_mut()[bb].statements.retain(|statement| {
             match &statement.kind {
                 StatementKind::Assign(box(place, _)) => {
                     if let Some(index) = place.as_local() {
@@ -1120,7 +1121,7 @@ pub fn promote_candidates<'tcx>(
                 _ => true
             }
         });
-        let terminator = block.terminator_mut();
+        let terminator = body.basic_block_terminator_mut(bb);
         match &terminator.kind {
             TerminatorKind::Drop { location: place, target, .. } => {
                 if let Some(index) = place.as_local() {
diff --git a/src/librustc_mir/transform/remove_noop_landing_pads.rs b/src/librustc_mir/transform/remove_noop_landing_pads.rs
@@ -103,15 +103,15 @@ impl RemoveNoopLandingPads {
         let postorder: Vec<_> = traversal::postorder(body).map(|(bb, _)| bb).collect();
         for bb in postorder {
             debug!("  processing {:?}", bb);
-            for target in body[bb].terminator_mut().successors_mut() {
+            for target in body.basic_block_terminator_mut(bb).successors_mut() {
                 if *target != resume_block && nop_landing_pads.contains(*target) {
                     debug!("    folding noop jump to {:?} to resume block", target);
                     *target = resume_block;
                     jumps_folded += 1;
                 }
             }
 
-            match body[bb].terminator_mut().unwind_mut() {
+            match body.basic_block_terminator_mut(bb).unwind_mut() {
                 Some(unwind) => {
                     if *unwind == Some(resume_block) {
                         debug!("    removing noop landing pad");
diff --git a/src/librustc_mir/transform/simplify.rs b/src/librustc_mir/transform/simplify.rs
diff --git a/src/librustc_mir/transform/simplify_branches.rs b/src/librustc_mir/transform/simplify_branches.rs
diff --git a/src/librustc_mir/util/patch.rs b/src/librustc_mir/util/patch.rs

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@ impl<'a, 'tcx> Iterator for Preorder<'a, 'tcx> {`
`55`	`55`
`56`	`56`	`let data = &self.body[idx];`
`57`	`57`
`58`		`- if let Some(ref term) = data.terminator {`
	`58`	`+ if let Some(ref term) = data.terminator_opt() {`
`59`	`59`	`self.worklist.extend(term.successors());`
`60`	`60`	`}`
`61`	`61`
`@@ -117,7 +117,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> {`
`117`	`117`
`118`	`118`	`let data = &po.body[root];`
`119`	`119`
`120`		`- if let Some(ref term) = data.terminator {`
	`120`	`+ if let Some(ref term) = data.terminator_opt() {`
`121`	`121`	`po.visited.insert(root);`
`122`	`122`	`po.visit_stack.push((root, term.successors()));`
`123`	`123`	`po.traverse_successor();`
`@@ -186,7 +186,7 @@ impl<'a, 'tcx> Postorder<'a, 'tcx> {`
`186`	`186`	`};`
`187`	`187`
`188`	`188`	`if self.visited.insert(bb) {`
`189`		`- if let Some(term) = &self.body[bb].terminator {`
	`189`	`+ if let Some(term) = &self.body[bb].terminator_opt() {`
`190`	`190`	`self.visit_stack.push((bb, term.successors()));`
`191`	`191`	`}`
`192`	`192`	`}`
Original file line number	Diff line number	Diff line change
`@@ -793,6 +793,7 @@ macro_rules! make_mir_visitor {`
`793`	`793`	`fn visit_location(&mut self, body: & $($mutability)? Body<'tcx>, location: Location) {`
`794`	`794`	`let basic_block = & $($mutability)? body[location.block];`
`795`	`795`	`if basic_block.statements.len() == location.statement_index {`
	`796`	`+ // TODO(nashenas88) how to ensure we clear the cache only in the mutable case...`
`796`	`797`	`if let Some(ref $($mutability)? terminator) = basic_block.terminator {`
`797`	`798`	`self.visit_terminator(terminator, location)`
`798`	`799`	`}`
Original file line number	Diff line number	Diff line change
`@@ -497,7 +497,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {`
`497`	`497`	`..`
`498`	`498`	`},`
`499`	`499`	`..`
`500`		`- }) = bbd.terminator {`
	`500`	`+ }) = bbd.terminator_opt() {`
`501`	`501`	`if let Some(source)`
`502`	`502`	`= BorrowedContentSource::from_call(func.ty(self.body, tcx), tcx)`
`503`	`503`	`{`
Original file line number	Diff line number	Diff line change
`@@ -731,7 +731,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {`
`731`	`731`
`732`	`732`	`fn finish(self) -> Body<'tcx> {`
`733`	`733`	`for (index, block) in self.cfg.basic_blocks.iter().enumerate() {`
`734`		`- if block.terminator.is_none() {`
	`734`	`+ if block.terminator_opt().is_none() {`
`735`	`735`	`span_bug!(self.fn_span, "no terminator on block {:?}", index);`
`736`	`736`	`}`
`737`	`737`	`}`