Mir predecessors cache invalidate on terminators #64841

Nashenas88 · 2019-09-27T13:20:14Z

No description provided.

… predecessors_for to slice

…k, invalidate cache when accessing unique terminator

rust-highfive · 2019-09-27T13:20:18Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

Nashenas88 · 2019-09-27T13:20:28Z

r? @oli-obk

Nashenas88 · 2019-09-27T13:21:02Z

Not sure if it makes sense to have 2 branches, but I have it separate in case we want to explore another option. The base is shared with #64736

Nashenas88 · 2019-09-27T13:22:46Z

src/librustc/mir/mod.rs

+
+    pub fn basic_block_terminator_mut(&mut self, bb: BasicBlock) -> &mut Terminator<'tcx> {
+        self.predecessors_cache = None;
+/*


This was my first attempt at "smart" invalidation, though I doubted it quickly after writing it. I don't get the feeling that this will be faster than just wiping the whole cache since the recreation still has to re-walk everything. Maybe if we have a way to mark some basic blocks as "dirty" then the recreation could be faster. I'll either update this or remove it before moving the PR out of draft.

I don't think we should be doing smart invalidation for now. We can leave that to the user and make them choose the appropriate function for their use case (thus not invalidating so much)

Nashenas88 · 2019-09-27T13:28:32Z

src/librustc/mir/mod.rs

@@ -1336,6 +1383,10 @@ impl<'tcx> BasicBlockData<'tcx> {
        BasicBlockData { statements: vec![], terminator, is_cleanup: false }
    }

+    pub fn terminator_opt(&self) -> &Option<Terminator<'tcx>> {
+        &self.terminator


Since I can't leave a comment outside of my changes...

I really wanted to make BasicBlockData::terminator private, but that's not possible because Cfg needs it in order to build up the BasicBlockData initially. Would it make sense to make terminator private and add a new method called something like terminator_for_cfg and terminator_for_cfg_mut that's intended only to be used by Cfg? This would help ensure that all other mutable access to terminator properly goes through Body so the cache can be properly maintained. In the current form, it would be very easy for someone to access &mut terminator directly without realizing it.

yea, seems fine to makeit private and create a BasicBlockData::new function that takes all values needed to construct a BasicBlockData

Nashenas88 · 2019-09-27T13:29:07Z

src/librustc/mir/mod.rs

        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> {


Is this safe to do? TerminatorKind doesn't affect successors, right?

TerminatorKind contains many BasicBlock ids, and if you change them, the successors change

rust-highfive · 2019-09-27T13:42:01Z

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-09-27T13:20:53.3281238Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-27T13:20:53.3471597Z ##[command]git config gc.auto 0
2019-09-27T13:20:54.1281247Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-27T13:20:54.1295706Z ##[command]git config --get-all http.proxy
2019-09-27T13:20:54.1303564Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64841/merge:refs/remotes/pull/64841/merge
---
2019-09-27T13:27:49.3376822Z    Compiling serde_json v1.0.40
2019-09-27T13:27:51.2821188Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-09-27T13:28:02.8011993Z     Finished release [optimized] target(s) in 1m 33s
2019-09-27T13:28:02.8125587Z tidy check
2019-09-27T13:28:03.2751102Z tidy error: /checkout/src/librustc_mir/transform/simplify.rs:280: line longer than 100 chars
2019-09-27T13:28:03.6545743Z tidy error: /checkout/src/librustc/mir/visit.rs:850: TODO is deprecated; use FIXME
2019-09-27T13:28:03.6577318Z tidy error: /checkout/src/librustc/mir/mod.rs:216: line longer than 100 chars
2019-09-27T13:28:03.6579212Z tidy error: /checkout/src/librustc/mir/mod.rs:278: TODO is deprecated; use FIXME
2019-09-27T13:28:04.9639765Z some tidy checks failed
2019-09-27T13:28:04.9645447Z 
2019-09-27T13:28:04.9645447Z 
2019-09-27T13:28:04.9646685Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-09-27T13:28:04.9646928Z 
2019-09-27T13:28:04.9647316Z 
2019-09-27T13:28:04.9660500Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-09-27T13:28:04.9660581Z Build completed unsuccessfully in 0:01:36
2019-09-27T13:28:04.9660581Z Build completed unsuccessfully in 0:01:36
2019-09-27T13:28:04.9711283Z == clock drift check ==
2019-09-27T13:28:04.9768355Z   local time: Fri Sep 27 13:28:04 UTC 2019
2019-09-27T13:28:05.1262633Z   network time: Fri, 27 Sep 2019 13:28:05 GMT
2019-09-27T13:28:05.1268739Z == end clock drift check ==
2019-09-27T13:28:06.6398448Z ##[error]Bash exited with code '1'.
2019-09-27T13:28:06.6445118Z ##[section]Starting: Checkout
2019-09-27T13:28:06.6447118Z ==============================================================================
2019-09-27T13:28:06.6447199Z Task         : Get sources
2019-09-27T13:28:06.6447248Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

oli-obk · 2019-09-27T13:45:29Z

src/librustc/mir/mod.rs

@@ -210,17 +210,38 @@ impl<'tcx> Body<'tcx> {

    #[inline]
    pub fn basic_blocks_mut(&mut self) -> &mut IndexVec<BasicBlock, BasicBlockData<'tcx>> {


this should still invalidate, or better not exist at all anymore

oli-obk · 2019-09-27T13:46:08Z

src/librustc/mir/mod.rs

        &mut self.basic_blocks
    }

+    pub fn basic_block_terminator_opt_mut(&mut self, bb: BasicBlock) -> &mut Option<Terminator<'tcx>> {


this could just be called terminator_opt_mut

oli-obk · 2019-09-27T13:47:18Z

src/librustc/mir/mod.rs

+
+    pub fn basic_block_terminator_mut(&mut self, bb: BasicBlock) -> &mut Terminator<'tcx> {
+        self.predecessors_cache = None;
+/*


I don't think we should be doing smart invalidation for now. We can leave that to the user and make them choose the appropriate function for their use case (thus not invalidating so much)

oli-obk · 2019-09-27T13:47:42Z

src/librustc/mir/mod.rs

    #[inline]
    pub fn basic_blocks_and_local_decls_mut(
        &mut self,
    ) -> (&mut IndexVec<BasicBlock, BasicBlockData<'tcx>>, &mut LocalDecls<'tcx>) {
-        self.predecessors_cache = None;


this still needs to invalidate, otherwise the cache is wrong.

oli-obk · 2019-09-27T13:49:08Z

src/librustc/mir/mod.rs

@@ -1336,6 +1383,10 @@ impl<'tcx> BasicBlockData<'tcx> {
        BasicBlockData { statements: vec![], terminator, is_cleanup: false }
    }

+    pub fn terminator_opt(&self) -> &Option<Terminator<'tcx>> {
+        &self.terminator


yea, seems fine to makeit private and create a BasicBlockData::new function that takes all values needed to construct a BasicBlockData

oli-obk · 2019-09-27T13:49:45Z

src/librustc/mir/mod.rs

        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> {


TerminatorKind contains many BasicBlock ids, and if you change them, the successors change

Nashenas88 added 4 commits September 24, 2019 09:09

Remove interior mutability in mir predecessors cache

187cd2a

Inline cache impl into Body, rename predecessor fns, change output of…

948328a

… predecessors_for to slice

Get rid of old comment

bea419e

Stop invalidating predecessors cache when accessing unique basic bloc…

e20d1f0

…k, invalidate cache when accessing unique terminator

rust-highfive assigned eddyb Sep 27, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2019

rust-highfive assigned oli-obk and unassigned eddyb Sep 27, 2019

Nashenas88 commented Sep 27, 2019

View reviewed changes

oli-obk reviewed Sep 27, 2019

View reviewed changes

Nashenas88 closed this Sep 28, 2019

Nashenas88 deleted the mir_predecessors_cache_invalidate_on_terminators branch September 28, 2019 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mir predecessors cache invalidate on terminators #64841

Mir predecessors cache invalidate on terminators #64841

Nashenas88 commented Sep 27, 2019

rust-highfive commented Sep 27, 2019

Nashenas88 commented Sep 27, 2019

Nashenas88 commented Sep 27, 2019

Nashenas88 Sep 27, 2019

oli-obk Sep 27, 2019

Nashenas88 Sep 27, 2019

oli-obk Sep 27, 2019

Nashenas88 Sep 27, 2019

oli-obk Sep 27, 2019

rust-highfive commented Sep 27, 2019

oli-obk Sep 27, 2019

oli-obk Sep 27, 2019

oli-obk Sep 27, 2019

oli-obk Sep 27, 2019

oli-obk Sep 27, 2019

oli-obk Sep 27, 2019

		@@ -210,17 +210,38 @@ impl<'tcx> Body<'tcx> {

		#[inline]
		pub fn basic_blocks_mut(&mut self) -> &mut IndexVec<BasicBlock, BasicBlockData<'tcx>> {

Mir predecessors cache invalidate on terminators #64841

Mir predecessors cache invalidate on terminators #64841

Conversation

Nashenas88 commented Sep 27, 2019

rust-highfive commented Sep 27, 2019

Nashenas88 commented Sep 27, 2019

Nashenas88 commented Sep 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-highfive commented Sep 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment