Stream: t-compiler/wg-mir-opt

Topic: Parallel rustc - Mir predecessors cache


Paul Faria (Sep 18 2019 at 13:28, on Zulip):

@oli this is related to the mir cache predecessors topic in #t-compiler/wg-parallel-rustc. So I think your idea of the separate query makes a lot of sense. I only didn't understand the part about the passes that need mutable access to the cache will have access anyways. Here are you implying that only the passes that generate the cache will need mutable access?

While reviewing the code last week, I saw that the only mutable access currently implemented is to simply wipe the cache when the basic blocks are updated, and the next read recomputes the cache.

oli (Sep 18 2019 at 13:30, on Zulip):

I checked the read sites, and I think all of them are either after after the optimized_mir query is done, or during optimizations

Paul Faria (Sep 18 2019 at 13:30, on Zulip):

How would that fit in with separate passes? Would this simply mean that a modification of the cache and a following read would cause the pass to be run again? (Which I thought was not possible since pass results are immutably cached) or does this never actually occur? I'm also not sure if this still holds when the compiler is processing the queries in parallel

oli (Sep 18 2019 at 13:30, on Zulip):

Separating the queries is thus unnecessary I believe

Paul Faria (Sep 18 2019 at 13:30, on Zulip):

Oh you just answered my second question :sweat_smile:

oli (Sep 18 2019 at 13:31, on Zulip):

we can simply remove all interior mutability and panic if it's still None when needed

Paul Faria (Sep 18 2019 at 13:33, on Zulip):

I'm not sure I'm following 100%. You're saying we should disallow invalidation of the cache?

Paul Faria (Sep 18 2019 at 13:36, on Zulip):

Actually, let me follow up when I'm back to my laptop.

oli (Sep 18 2019 at 13:38, on Zulip):

ah, no, invalidation can only happen while we have mutable access, because invalidation happens when mutating the body

oli (Sep 18 2019 at 13:39, on Zulip):

so we can just set it to None, but that doesn't need interior mutability

oli (Sep 18 2019 at 13:39, on Zulip):

whenever we need it and have mutable access, we can reinitialize it (or just read it if it's Some)

oli (Sep 18 2019 at 13:40, on Zulip):

so nothing changes about the status quo, except that we have two "read" accessors

oli (Sep 18 2019 at 13:40, on Zulip):

one with &mut that initializes if necessary

oli (Sep 18 2019 at 13:40, on Zulip):

and one with & that panics if None

oli (Sep 18 2019 at 13:41, on Zulip):

we'll also need a dummy pass at the end of the pipeline that does nothing but initialize the cache

oli (Sep 18 2019 at 13:41, on Zulip):

after optimizations it will be needed at least once, so we don't lose anything by computing the cache eagerly

Paul Faria (Sep 18 2019 at 21:53, on Zulip):

Sorry for the delay. Traveling to Sunnyvale today and had some surprises pop up in the morning. I might be able to start serious work on this Friday and if not then sometime in the weekend.

oli (Sep 19 2019 at 13:40, on Zulip):

no pressure! Take all the time you need

Paul Faria (Sep 22 2019 at 21:14, on Zulip):

So I implemented your suggestion, but I still feel worried about this approach (still compiling at the moment, but I'm pretty sure this is going to crash). All readers of the predecessors only ever wanted immutable read access. However, the code that invalidates the predecessors happens more frequently. I did a regex search against basic_body_.*?mut and found with 27 instances that would invalidate the predecessors cache. The other issue I ran into is that it seems interior mutability was being used to avoid a clone when requesting the predecessors. It was being called as self.cache.predecessors(self), where self was of type &Body<...>. When changing this to not use interior mutability, that means I need cache to be &mut self in predecessors_mut, but this requires that self: &mut Body<...>. That won't compile because the passed self is already borrowed. I can get around this by cloning self: &Body<...>, but that doesn't seem ideal for performance.

Paul Faria (Sep 22 2019 at 21:16, on Zulip):

I think what I'll do later tonight (have to step out for a few hours) if not tomorrow evening, is try to manually map out where the cache is being invalidated and where's it's being read (and would possibly require unique access to the cache to recompute).

Paul Faria (Sep 22 2019 at 21:19, on Zulip):

There were also notes left in the comments about maybe implementing a more intelligent cache that only cleared what was needed. Do you think now might be a good opportunity to revisit that idea, or only once the interior mutability is all cleared up?

Paul Faria (Sep 22 2019 at 21:20, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc/mir/cache.rs#L42 is the comment I'm referring to.

Paul Faria (Sep 23 2019 at 13:13, on Zulip):

@oli whenever you're free ^. Side question: what's the etiquette for pinging people in Zulip? I wasn't sure if pinging every time I had a question would be too annoying, or useful as a way to focus someone where they're needed

Paul Faria (Sep 24 2019 at 02:07, on Zulip):

@oli I found one spot where the predecessors are read and then immediately written over (the mut access will invalidate the cache):
https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/add_call_guards.rs#L42-L49

Paul Faria (Sep 24 2019 at 02:08, on Zulip):

Is it possible that other optimizations could be running at the same time this is being computed?

Paul Faria (Sep 24 2019 at 02:08, on Zulip):

Or is this an instance where I should be accessing the mutable variant of predecessors?

oli (Sep 24 2019 at 08:13, on Zulip):

pinging is fine (at least for me :D)

oli (Sep 24 2019 at 08:19, on Zulip):

So... multiple things:

  1. probably not all basic_block_mut calls actually mutate the terminator's targets or the number of blocks, maybe we should find a scheme for that
  2. Unfortunately I can't quite follow what you're saying about multiple mutable borrows, can you push some code that I can look at?
  3. the comment may be referring to 1. because otherwise I have no idea how to approach a partial invalidation
Paul Faria (Sep 24 2019 at 13:10, on Zulip):

I opened a draft PR here https://github.com/rust-lang/rust/pull/64736

Paul Faria (Sep 24 2019 at 13:12, on Zulip):

This is the line I can't figure out how to avoid a clone on https://github.com/rust-lang/rust/pull/64736/files#diff-f0577ac900ffbd36d3bb3421a928cbbdR234

Paul Faria (Sep 24 2019 at 13:13, on Zulip):

I'll think about options for 1. I need to run to work for the moment, but I should be back online late tonight to continue working through ideas. I'll follow up with you tomorrow morning

oli (Sep 24 2019 at 13:20, on Zulip):

I posted some comments on the PR

Paul Faria (Sep 24 2019 at 15:34, on Zulip):

I took a look. Moving the cache into Body makes a lot more sense than the idea I was sitting on :sweat_smile:

Paul Faria (Sep 24 2019 at 15:34, on Zulip):

I'll address those tonight

Paul Faria (Sep 25 2019 at 01:48, on Zulip):

@oli the latest changes are up. My only hesitation with the current impl is this: since the predecessors are only calculated on read of a mut Body, what guarantees that they're computed before they're being read, e.g. in nll? The scenario I'm thinking of is this: Most of the optimizations on MIR will invalidate the predecessors cache, since they request mutable access to the basic blocks, but what code has mutable access to a Body that guarantees that this cache is regenerated when needed? Is it possible we need some pass where we can ensure that the cache is complete and immutable for later parts of the compiler?

Paul Faria (Sep 25 2019 at 01:54, on Zulip):

I've also been thinking about a way to more intelligently update the cache. What if rather than doing a full wipe-out, we had a way to track which blocks were changed during a mutation event, and update the predecessors immediately after that block completed? E.g. something like (fair warning, I'm terrible at naming things on a first pass):

body.track_body_changes(|&mut basic_blocks| -> {
  basic_blocks.add_terminator(...) // gets tracked for cache
  basic_blocks.move_terminator(...) // gets tracked for cache
  basic_blocks.simplify_interior_of_block(..) // ignored so it's not touching terminators
}) // after the lambda executes, but before the fn returns, the predecessors cache is recomputed

This would ensure there's always a cache to look at, but would avoid updating it too frequently. It might also save us from the excessive invalidation that currently happens (invalidate is called really frequently). However, I'm not sure how this is any better than just adding a method calls for terminators specifically, that will add to a predecessors lookup. I.e., special methods for modifying terminators since those are the only ones that would affect predecessors.

oli (Sep 25 2019 at 07:36, on Zulip):

Is it possible we need some pass where we can ensure that the cache is complete and immutable for later parts of the compiler?

yes definitely, this must be the final pass

oli (Sep 25 2019 at 07:37, on Zulip):

wrt updating, I think a scheme similar to what you are suggesting is possible, though I worry whether it will get very complex if we want to truly prevent all irrelevant mutations from requiring cache recomputation

oli (Sep 25 2019 at 07:38, on Zulip):

e.g. think about modifying the conditions of a terminator. This does not affect the predecessor cache

oli (Sep 25 2019 at 07:39, on Zulip):

maybe we could start out with a body.block_statements_mut() function that does not invalidate and see how far that gets us?

Paul Faria (Sep 25 2019 at 13:22, on Zulip):

e.g. think about modifying the conditions of a terminator. This does not affect the predecessor cache

I think if we can figure out a way to only invalidate on terminator changes (and not on any basic block changes), that might still be useful. I agree there's definitely a point of diminishing returns that won't be useful to pursue.

Paul Faria (Sep 25 2019 at 13:31, on Zulip):

I did a quick search, and it seems terminators are only created/modified through BasicBlockData's terminator_mut (please correct me if I've missed something) and I only found 13 instances of this being used. I'm thinking maybe we can have those be managed by the Body type in some form so that the cache just for those specific terminators can be invalidated. I'll try playing around with that tonight. I'll also move it to a separate branch so we can do perf comparisons against any other ideas that come up.

oli (Sep 25 2019 at 13:34, on Zulip):

This fits into the scheme of having a statements_mut function

oli (Sep 25 2019 at 13:35, on Zulip):

(which doesn't invalidate)

Paul Faria (Sep 27 2019 at 13:30, on Zulip):

Latest changes are up here: https://github.com/rust-lang/rust/pull/64841
I made a new PR since I wasn't sure if we were committed to this specific change or not. If you think all of this should be in the same PR, I'll close this one and update the original one.

Paul Faria (Sep 27 2019 at 13:30, on Zulip):

I'm also still missing the final pass to ensure the creation of the cache before the read-only phases.

Paul Faria (Sep 27 2019 at 13:31, on Zulip):

Is it correct to assume I should add it to this list: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/mod.rs#L228

Paul Faria (Sep 27 2019 at 13:32, on Zulip):

Also, if this is the correct place to modify, would it go just before the dump pass, or do you think there's a more ideal location to run that kind of pass?

Paul Faria (Sep 27 2019 at 13:33, on Zulip):

@oli , whenever you're free ^

oli (Sep 27 2019 at 13:41, on Zulip):

yea it could definitely be infront of the dump pass

oli (Sep 27 2019 at 13:41, on Zulip):

I'll look at the PR in detail later

oli (Sep 27 2019 at 13:42, on Zulip):

huh :D why is there a second PR?

oli (Sep 27 2019 at 13:42, on Zulip):

ah you wrote that above

oli (Sep 27 2019 at 13:42, on Zulip):

yea do everything in the one PR

oli (Sep 27 2019 at 13:44, on Zulip):

hmm.. oh that's about the smart invalidation?

oli (Sep 27 2019 at 13:44, on Zulip):

yea that shouldn't be intermingled

oli (Sep 27 2019 at 13:44, on Zulip):

The first PR shouldn't optimize the invalidation

oli (Sep 27 2019 at 13:44, on Zulip):

just keeping the status quo is ok

Paul Faria (Sep 27 2019 at 13:56, on Zulip):

Ok, I'll get to that at lunch time. I assume you'll be reviewing that Monday due to time zones?

oli (Sep 27 2019 at 13:58, on Zulip):

maybe XD sometimes I find time on weekends, too

oli (Sep 27 2019 at 13:58, on Zulip):

did you change anything in the root PR?

oli (Sep 27 2019 at 13:59, on Zulip):

IIRC that just needs the mir pass regenerating the cache and it's done keeping the status quo alive

Paul Faria (Sep 27 2019 at 15:28, on Zulip):

In the root PR I haven't updated anything. You're saying just add the pass and then continue on this next PR?

oli (Sep 28 2019 at 10:05, on Zulip):

yes

Paul Faria (Sep 28 2019 at 15:30, on Zulip):

Ok, I think I've addressed everything. I also added comments on the PR about a couple areas I think could be improved as well as one single place I'm not sure how to enforce the use of the terminator lookup since it's inside of a macro that takes in the mutability during the call.

Paul Faria (Sep 28 2019 at 19:41, on Zulip):

So I started looking at why the CI was failing. Some lint checks failed, so I got those through, but now I'm running into one area that looks like it's going to explode in complexity without interior mutability. So currently, there's a predecessors_for fn that called unwrap_predecessors (it's one that I had a TODO on about whether that was the correct *predecessors fn to call). predecessors_for is called by a trait impl impl <Body as graph::WithPredecessors>::predecessors. The first time this is called, the cache hasn't been computed, so the compiler panics. Changing this to use predecessors (which computes the cache) isn't possible because once we get to the graph::WithPredecessors trait, we're bound to use &self. Assuming this even made sense (which I don't think it does), I could fix this instance by changing WithPredecessors::predecessor to take &mut self, but that requires changing all other impls of that trait.

Paul Faria (Sep 30 2019 at 13:18, on Zulip):

@oli whenever you're free, see the thread above

oli (Sep 30 2019 at 13:18, on Zulip):

yea :D the email notification about your PR is next on my list of things to process

oli (Sep 30 2019 at 13:19, on Zulip):

I'll look at it after work

Paul Faria (Sep 30 2019 at 13:20, on Zulip):

No rush, though the comment I have above is not mentioned in the PR. It's an overview of my review of the CI failures

oli (Sep 30 2019 at 17:37, on Zulip):

There's only a single impl of WithPredecessors (not counting tests), so yea, feel free to change the trait

oli (Sep 30 2019 at 17:37, on Zulip):

Assuming this even made sense (which I don't think it does),

oli (Sep 30 2019 at 17:37, on Zulip):

what do you mean by this?

oli (Sep 30 2019 at 17:37, on Zulip):

canyou elaborate?

Paul Faria (Sep 30 2019 at 19:42, on Zulip):

Oh I didn't realize the other implementors were tests. I thought it was other parts of the compiler, and making all of them mut didn't make sense to me

Paul Faria (Sep 30 2019 at 19:42, on Zulip):

Sorry I missed that

Paul Faria (Sep 30 2019 at 22:01, on Zulip):

@oli just got a chance to take a look again, and it's not just the tests. librustc_data_structures/graph also implements this trait, and trying to fix this is causing a trickling of errors that I have to keep fixing.

oli (Oct 01 2019 at 06:41, on Zulip):

hmm my grep foo must have failed me

oli (Oct 01 2019 at 06:41, on Zulip):

I thought the other uses were just generic forwardings

oli (Oct 01 2019 at 06:42, on Zulip):

maybe make the trait work on self instead of &self or &mut self and implement it for &Foo and &mut Bar

Paul Faria (Oct 01 2019 at 11:55, on Zulip):

All this time and I never even knew you could do that... Ok I'll work on that this morning. Thanks!

Paul Faria (Oct 01 2019 at 13:22, on Zulip):

So I was confused for a bit until I took a step back and reviewed how all of the code is connected. Your suggestion won't work because I hadn't realized that the methods in librustc_data_structures are actually being called against & &mut Body (after my changes, previously, it was &&Body where &Body was abstracted behind some generic type G), but they're being made to work against any type that implements ControlFlowGraph, which all expect some &T. Implementing WithPredecessors::predecessor(self, ...) would require me to convert &&Body to &mut Body.

oli (Oct 01 2019 at 13:29, on Zulip):

:frown:

oli (Oct 01 2019 at 13:30, on Zulip):

I guess just refreshing the cache before calling the WithPredecessors thing isn't an option?

Paul Faria (Oct 01 2019 at 13:34, on Zulip):

e_O I hadn't thought of that... let me see how often that's called

Paul Faria (Oct 01 2019 at 13:58, on Zulip):

Ok, that helped to move past that panic. Running into other ones now. I'll see if I can't try the same approach

Paul Faria (Oct 01 2019 at 13:59, on Zulip):

Actually, what's a little confusing to me is that the next crash is in the codegen passes. Wouldn't the final pass I added inside of optimize passes have fixed that?

oli (Oct 01 2019 at 14:08, on Zulip):

it should have fixed that indeed, do you have a backtrace?

Paul Faria (Oct 01 2019 at 14:32, on Zulip):

Yep, let's see if it's not too long for zulip (one sec while I copy paste from the terminal)

Paul Faria (Oct 01 2019 at 14:32, on Zulip):
thread 'rustc' panicked at 'assertion failed: self.predecessors_cache.is_some()', src/librustc/mir/mod.rs:239:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:76
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:60
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1030
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1412
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:64
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:196
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:210
  10: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
             at ./src/liballoc/boxed.rs:936
  11: rustc_driver::report_ice
             at src/librustc_driver/lib.rs:1188
  12: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:477
  13: std::panicking::begin_panic
             at ./src/libstd/panicking.rs:407
  14: rustc::mir::Body::unwrap_predecessors
             at ./<::std::macros::panic macros>:3
  15: rustc::mir::Body::predecessors_for
             at ./src/librustc/mir/mod.rs:268
  16: rustc_codegen_ssa::mir::codegen_mir
             at ./src/librustc_codegen_ssa/mir/mod.rs:210
  17: rustc_codegen_ssa::base::codegen_instance
             at ./src/librustc_codegen_ssa/base.rs:386
  18: <rustc::mir::mono::MonoItem as rustc_codegen_ssa::mono_item::MonoItemExt>::define
             at ./src/librustc_codegen_ssa/mono_item.rs:40
  19: rustc_codegen_llvm::base::compile_codegen_unit::module_codegen
             at src/librustc_codegen_llvm/base.rs:143
  20: rustc::dep_graph::graph::DepGraph::with_task_impl
             at ./src/librustc/dep_graph/graph.rs:334
  21: rustc::dep_graph::graph::DepGraph::with_task
             at ./src/librustc/dep_graph/graph.rs:202
  22: rustc_codegen_llvm::base::compile_codegen_unit
             at src/librustc_codegen_llvm/base.rs:110
  23: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_ssa::traits::backend::ExtraBackendMethods>::compile_codegen_unit
             at src/librustc_codegen_llvm/lib.rs:126
  24: rustc_codegen_ssa::base::codegen_crate
             at ./src/librustc_codegen_ssa/base.rs:617
  25: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_utils::codegen_backend::CodegenBackend>::codegen_crate
             at src/librustc_codegen_llvm/lib.rs:289
  26: rustc_interface::passes::start_codegen::{{closure}}
             at src/librustc_interface/passes.rs:1092
  27: rustc::util::common::time_ext
             at ./src/librustc/util/common.rs:117
  28: rustc::util::common::time
             at ./src/librustc/util/common.rs:111
  29: rustc_interface::passes::start_codegen
             at src/librustc_interface/passes.rs:1091
  30: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::ongoing_codegen::{{closure}}::{{closure}}
             at src/librustc_interface/queries.rs:243
  31: rustc_interface::passes::BoxedGlobalCtxt::enter::{{closure}}::{{closure}}
             at src/librustc_interface/passes.rs:813
  32: rustc::ty::context::tls::enter_global::{{closure}}
             at ./src/librustc/ty/context.rs:1883
  33: rustc::ty::context::tls::enter_context::{{closure}}
             at ./src/librustc/ty/context.rs:1851
  34: rustc::ty::context::tls::set_tlv
             at ./src/librustc/ty/context.rs:1784
  35: rustc::ty::context::tls::enter_context
             at ./src/librustc/ty/context.rs:1850
  36: rustc::ty::context::tls::enter_global
             at ./src/librustc/ty/context.rs:1882
  37: rustc_interface::passes::BoxedGlobalCtxt::enter::{{closure}}
             at src/librustc_interface/passes.rs:813
  38: rustc_interface::passes::BoxedGlobalCtxt::access::{{closure}}
             at ./<::rustc_data_structures::box_region::declare_box_region_type macros>:22
  39: rustc_interface::passes::create_global_ctxt::{{closure}}
             at src/librustc_interface/passes.rs:879
  40: alloc::boxed::<impl core::ops::generator::Generator for core::pin::Pin<alloc::boxed::Box<G>>>::resume
             at ./src/liballoc/boxed.rs:1073
  41: rustc_data_structures::box_region::PinnedGenerator<I,A,R>::access
             at ./src/librustc_data_structures/box_region.rs:52
  42: rustc_interface::passes::BoxedGlobalCtxt::access
             at ./<::rustc_data_structures::box_region::declare_box_region_type macros>:25
  43: rustc_interface::passes::BoxedGlobalCtxt::enter
             at src/librustc_interface/passes.rs:813
  44: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::ongoing_codegen::{{closure}}
             at src/librustc_interface/queries.rs:237
  45: rustc_interface::queries::Query<T>::compute
             at src/librustc_interface/queries.rs:28
  46: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::ongoing_codegen
             at src/librustc_interface/queries.rs:234
  47: rustc_driver::run_compiler::{{closure}}
             at src/librustc_driver/lib.rs:388
  48: rustc_interface::interface::run_compiler_in_existing_thread_pool
             at ./src/librustc_interface/interface.rs:122
  49: rustc_interface::interface::run_compiler::{{closure}}
             at ./src/librustc_interface/interface.rs:141
  50: rustc_interface::util::spawn_thread_pool::{{closure}}::{{closure}}::{{closure}}::{{closure}}
             at ./src/librustc_interface/util.rs:192
  51: rustc::ty::context::tls::with_thread_locals::{{closure}}::{{closure}}
             at ./src/librustc/ty/context.rs:1839
  52: std::thread::local::LocalKey<T>::try_with
             at ./src/libstd/thread/local.rs:262
  53: std::thread::local::LocalKey<T>::with
             at ./src/libstd/thread/local.rs:239
  54: rustc::ty::context::tls::with_thread_locals::{{closure}}
             at ./src/librustc/ty/context.rs:1831
  55: std::thread::local::LocalKey<T>::try_with
             at ./src/libstd/thread/local.rs:262
  56: std::thread::local::LocalKey<T>::with
             at ./src/libstd/thread/local.rs:239
  57: rustc::ty::context::tls::with_thread_locals
             at ./src/librustc/ty/context.rs:1823
  58: rustc_interface::util::spawn_thread_pool::{{closure}}::{{closure}}::{{closure}}
             at ./src/librustc_interface/util.rs:192
  59: scoped_tls::ScopedKey<T>::set
             at /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-1.0.0/src/lib.rs:137
  60: rustc_interface::util::spawn_thread_pool::{{closure}}::{{closure}}
             at ./src/librustc_interface/util.rs:188
  61: scoped_tls::ScopedKey<T>::set
             at /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-1.0.0/src/lib.rs:137
  62: syntax::with_globals::{{closure}}
             at ./src/libsyntax/lib.rs:109
  63: scoped_tls::ScopedKey<T>::set
             at /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-1.0.0/src/lib.rs:137
  64: syntax::with_globals
             at ./src/libsyntax/lib.rs:108
  65: rustc_interface::util::spawn_thread_pool::{{closure}}
             at ./src/librustc_interface/util.rs:187
  66: rustc_interface::util::scoped_thread::{{closure}}
             at ./src/librustc_interface/util.rs:164
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
oli (Oct 01 2019 at 14:46, on Zulip):

Oh. I think this is because the value comes from instance_mir, which may be an autogenerated MIR and not one that passed through optimized_mir

Paul Faria (Oct 01 2019 at 14:54, on Zulip):

Is that one generated with passes in a similar manner? Where I should add the predecessors pass at the end of that one as well?

Paul Faria (Oct 01 2019 at 14:54, on Zulip):

Or is it generated differently?

oli (Oct 01 2019 at 14:58, on Zulip):

you should be able to add your pass to https://github.com/rust-lang/rust/blob/22bc9e1d9ca49ee4f5cd953088ab09c238a6dd26/src/librustc_mir/shim.rs#L115

Paul Faria (Oct 01 2019 at 15:12, on Zulip):

That didn't seem to work. I tried adding a log to the pass, but I can't seem to get it to emit

Paul Faria (Oct 01 2019 at 15:12, on Zulip):

Is the env variable RUST_LOG=librustc_mir=debug? Or is it different when running in the compiler?

oli (Oct 01 2019 at 15:18, on Zulip):

I think you need RUSTC_LOG

Paul Faria (Oct 01 2019 at 15:22, on Zulip):

It also helps if I ask for librustc_mir rather than lubrustc_mir :sweat_smile:

oli (Oct 01 2019 at 15:22, on Zulip):

ha

oli (Oct 01 2019 at 15:23, on Zulip):

lobster_mir

oli (Oct 01 2019 at 15:23, on Zulip):

we should name our crates for rustaceans

Christian Poveda (Oct 01 2019 at 15:29, on Zulip):

lobster_mir

I'm going to register that into crates.io right now

Paul Faria (Oct 02 2019 at 02:55, on Zulip):

@oli I just got around to trying the changes out. Adding the pass in the shim didn't seem to make a difference. I added it at the end. Should it be done earlier? Also, here's a snippet of the logs I collected:

thread 'rustc' panicked at 'Expected predecessors_cache to be `Some(...)` at: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:19:5: 19:20', src/librustc/mir/mod.rs:241:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at ./src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at ./src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:76
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:60
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1030
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1412
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:64
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:196
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:210
  10: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
             at /home/pfaria/projects/rust/src/liballoc/boxed.rs:936
  11: rustc_driver::report_ice
             at src/librustc_driver/lib.rs:1188
  12: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:477
  13: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:380
  14: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:335
  15: rustc::mir::Body::unwrap_predecessors
             at /home/pfaria/projects/rust/<::std::macros::panic macros>:9
  16: rustc::mir::Body::predecessors_for
             at /home/pfaria/projects/rust/src/librustc/mir/mod.rs:271
  17: rustc_codegen_ssa::mir::codegen_mir
             at /home/pfaria/projects/rust/src/librustc_codegen_ssa/mir/mod.rs:210
  18: rustc_codegen_ssa::base::codegen_instance
             at /home/pfaria/projects/rust/src/librustc_codegen_ssa/base.rs:386
  19: <rustc::mir::mono::MonoItem as rustc_codegen_ssa::mono_item::MonoItemExt>::define
             at /home/pfaria/projects/rust/src/librustc_codegen_ssa/mono_item.rs:40
  20: rustc_codegen_llvm::base::compile_codegen_unit::module_codegen
             at src/librustc_codegen_llvm/base.rs:143
  21: rustc::dep_graph::graph::DepGraph::with_task_impl
             at /home/pfaria/projects/rust/src/librustc/dep_graph/graph.rs:334
  22: rustc::dep_graph::graph::DepGraph::with_task
             at /home/pfaria/projects/rust/src/librustc/dep_graph/graph.rs:202
  23: rustc_codegen_llvm::base::compile_codegen_unit
             at src/librustc_codegen_llvm/base.rs:110
  24: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_ssa::traits::backend::ExtraBackendMethods>::compile_codegen_unit
             at src/librustc_codegen_llvm/lib.rs:126
  25: rustc_codegen_ssa::base::codegen_crate
             at /home/pfaria/projects/rust/src/librustc_codegen_ssa/base.rs:617
  26: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_utils::codegen_backend::CodegenBackend>::codegen_crate
             at src/librustc_codegen_llvm/lib.rs:289
  27: rustc_interface::passes::start_codegen::{{closure}}
             at src/librustc_interface/passes.rs:1092
  28: rustc::util::common::time_ext
             at /home/pfaria/projects/rust/src/librustc/util/common.rs:117
  29: rustc::util::common::time
             at /home/pfaria/projects/rust/src/librustc/util/common.rs:111
  30: rustc_interface::passes::start_codegen
             at src/librustc_interface/passes.rs:1091
  31: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::ongoing_codegen::{{closure}}::{{closure}}
             at src/librustc_interface/queries.rs:243
  32: rustc_interface::passes::BoxedGlobalCtxt::enter::{{closure}}::{{closure}}
             at src/librustc_interface/passes.rs:813
  33: rustc::ty::context::tls::enter_global::{{closure}}
             at /home/pfaria/projects/rust/src/librustc/ty/context.rs:1883
  34: rustc::ty::context::tls::enter_context::{{closure}}
             at /home/pfaria/projects/rust/src/librustc/ty/context.rs:1851
  35: rustc::ty::context::tls::set_tlv
             at /home/pfaria/projects/rust/src/librustc/ty/context.rs:1784
  36: rustc::ty::context::tls::enter_context
             at /home/pfaria/projects/rust/src/librustc/ty/context.rs:1850
  37: rustc::ty::context::tls::enter_global
             at /home/pfaria/projects/rust/src/librustc/ty/context.rs:1882
  38: rustc_interface::passes::BoxedGlobalCtxt::enter::{{closure}}
             at src/librustc_interface/passes.rs:813
  39: rustc_interface::passes::BoxedGlobalCtxt::access::{{closure}}
             at /home/pfaria/projects/rust/<::rustc_data_structures::box_region::declare_box_region_type macros>:22
  40: rustc_interface::passes::create_global_ctxt::{{closure}}
             at src/librustc_interface/passes.rs:879
  41: alloc::boxed::<impl core::ops::generator::Generator for core::pin::Pin<alloc::boxed::Box<G>>>::resume
             at /home/pfaria/projects/rust/src/liballoc/boxed.rs:1073
  42: rustc_data_structures::box_region::PinnedGenerator<I,A,R>::access
             at /home/pfaria/projects/rust/src/librustc_data_structures/box_region.rs:52
  43: rustc_interface::passes::BoxedGlobalCtxt::access
             at /home/pfaria/projects/rust/<::rustc_data_structures::box_region::declare_box_region_type macros>:25
  44: rustc_interface::passes::BoxedGlobalCtxt::enter
             at src/librustc_interface/passes.rs:813
  45: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::ongoing_codegen::{{closure}}
             at src/librustc_interface/queries.rs:237
  46: rustc_interface::queries::Query<T>::compute
             at src/librustc_interface/queries.rs:28
  47: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::ongoing_codegen
             at src/librustc_interface/queries.rs:234
  48: rustc_driver::run_compiler::{{closure}}
             at src/librustc_driver/lib.rs:388
  49: rustc_interface::interface::run_compiler_in_existing_thread_pool
             at /home/pfaria/projects/rust/src/librustc_interface/interface.rs:122
  50: rustc_interface::interface::run_compiler::{{closure}}
             at /home/pfaria/projects/rust/src/librustc_interface/interface.rs:141
  51: rustc_interface::util::spawn_thread_pool::{{closure}}::{{closure}}::{{closure}}::{{closure}}
             at /home/pfaria/projects/rust/src/librustc_interface/util.rs:192
  52: rustc::ty::context::tls::with_thread_locals::{{closure}}::{{closure}}
             at /home/pfaria/projects/rust/src/librustc/ty/context.rs:1839
  53: std::thread::local::LocalKey<T>::try_with
             at /home/pfaria/projects/rust/src/libstd/thread/local.rs:262
  54: std::thread::local::LocalKey<T>::with
             at /home/pfaria/projects/rust/src/libstd/thread/local.rs:239
  55: rustc::ty::context::tls::with_thread_locals::{{closure}}
             at /home/pfaria/projects/rust/src/librustc/ty/context.rs:1831
  56: std::thread::local::LocalKey<T>::try_with
             at /home/pfaria/projects/rust/src/libstd/thread/local.rs:262
  57: std::thread::local::LocalKey<T>::with
             at /home/pfaria/projects/rust/src/libstd/thread/local.rs:239
  58: rustc::ty::context::tls::with_thread_locals
             at /home/pfaria/projects/rust/src/librustc/ty/context.rs:1823
  59: rustc_interface::util::spawn_thread_pool::{{closure}}::{{closure}}::{{closure}}
             at /home/pfaria/projects/rust/src/librustc_interface/util.rs:192
  60: scoped_tls::ScopedKey<T>::set
             at /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-1.0.0/src/lib.rs:137
  61: rustc_interface::util::spawn_thread_pool::{{closure}}::{{closure}}
             at /home/pfaria/projects/rust/src/librustc_interface/util.rs:188
  62: scoped_tls::ScopedKey<T>::set
             at /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-1.0.0/src/lib.rs:137
  63: syntax::with_globals::{{closure}}
             at /home/pfaria/projects/rust/src/libsyntax/lib.rs:109
  64: scoped_tls::ScopedKey<T>::set
             at /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/scoped-tls-1.0.0/src/lib.rs:137
  65: syntax::with_globals
             at /home/pfaria/projects/rust/src/libsyntax/lib.rs:108
  66: rustc_interface::util::spawn_thread_pool::{{closure}}
             at /home/pfaria/projects/rust/src/librustc_interface/util.rs:187
  67: rustc_interface::util::scoped_thread::{{closure}}
             at /home/pfaria/projects/rust/src/librustc_interface/util.rs:164
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Paul Faria (Oct 02 2019 at 02:55, on Zulip):

And the logs I had from before that:

[DEBUG rustc::mir] PAUL: Recomputing predecessors: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Invalidating through opt terminator: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc_mir::transform::ensure_predecessors_cache] before-opt-dump: Ensure predecessors cache: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
[DEBUG rustc::mir] PAUL: Recomputing predecessors: /home/pfaria/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/types.rs:16:10: 16:15
...
Paul Faria (Oct 02 2019 at 02:56, on Zulip):

So it seems to be processing the backtrace crate (I doubt this is important), but the crash occurs when looking at the body for types.rs:19:5: 19:20, which is never logged at any point previously during compilation (so no cache computing, not invalidation attempts)

Paul Faria (Oct 02 2019 at 03:12, on Zulip):

I have the full trace as well, if that would help, but it's ~27,000 lines long and includes a lot of extra logs. I couldn't figure out how to filter to only log at the rustc::mir level and nothing below that

oli (Oct 02 2019 at 06:17, on Zulip):

maybe there's another way instance_mir can generate MIR that I haven't seen? Try following all the function calls in instance_mir and below

Paul Faria (Oct 02 2019 at 12:54, on Zulip):

Oooh, Looks like there might be a special case for tuple struct constructors. Compiling an attempt now

Paul Faria (Oct 02 2019 at 13:16, on Zulip):

Ok, got past that panic, on to the next few. I'll review the next ones after work. Thanks again!

oli (Oct 02 2019 at 13:21, on Zulip):

:D whack-a-panic

Paul Faria (Oct 03 2019 at 03:51, on Zulip):

@oli: It can finally compile the compiler :). In my analysis I realized the terminator_mut method idea I had gets called a LOT, so I'd like to do a comparison between that and the previous one (I think I might have messed up your earlier instructions about splitting up the PR, this PR include a different cache invalidation strategy). The latest commit is as small as I thought I could make it

Paul Faria (Oct 04 2019 at 04:40, on Zulip):

I addressed a lot of the issues in the draft PR. I'm rolling back the changes that made terminator invalidate, but I'm seeing that the cache is invalidated more frequently now, so there are additional panics I'm working through. Going to follow up tomorrow morning

nikomatsakis (Oct 04 2019 at 12:34, on Zulip):

Nice

Paul Faria (Oct 04 2019 at 13:47, on Zulip):

@oli I addressed the additional comments you had. For some reason I cannot comment on or resolve the comment you left on src/librustc_mir/transform/add_call_guards.rs Outdated. I think for this first pass the only remaining item to address is how to manage the serialization of that field (it's the comment midway through the PR from last week).

Paul Faria (Oct 04 2019 at 13:52, on Zulip):

Also, would it make sense to move it out of draft at this point?

Paul Faria (Oct 07 2019 at 21:36, on Zulip):

I added the cache class back so we can ensure there's no encoding/decoding of the cache going on. This broke a few more things. However, now that the compiler is passing, a bunch of tests are panicking. I'll try to address those as well. It does feel a bit scary though that I can't easily predict where the next panic will come from.

oli (Oct 08 2019 at 09:49, on Zulip):

that's super odd. I thought all the uses of the cache were in the same crate and not cross crate

Paul Faria (Oct 08 2019 at 10:55, on Zulip):

It was breaking on things like num::wrapping_add and array... Something. They were both generated by macro calls I think. I'll follow up when I get back to my computer

Paul Faria (Oct 08 2019 at 12:36, on Zulip):

Not sure why I put macros there. I shouldn't message immediately after waking up... It was getting the result from instance_mir, and there from optimized_mir. But I wasn't sure which impl it was calling because there are two in the compiler.. The weirdest part was that neither impl would log in the case of the panic, but they would in other instances.

Paul Faria (Oct 08 2019 at 12:41, on Zulip):

I might have also been jumping to conclusions about the encoding/decoding part. That was just the one part that would make sense given the only change at the time was the move back to the cache with the encoding/decoding changes.

Paul Faria (Oct 08 2019 at 12:47, on Zulip):

Also, inside of instance_mir, which optimized_mir is getting called? Is it https://github.com/rust-lang/rust/blob/e3cb9ea15a2082f39d4d4f10a22e779701dd0d64/src/librustc_metadata/cstore_impl.rs#L129 or https://github.com/rust-lang/rust/blob/22bc9e1d9ca49ee4f5cd953088ab09c238a6dd26/src/librustc_mir/transform/mod.rs#L282 ?

oli (Oct 08 2019 at 12:48, on Zulip):

the latter, but the latter may call the former

Paul Faria (Oct 08 2019 at 12:48, on Zulip):

I had logs at the beginning of both of those calls, as well as surrounding the call in instance_mir. Specifically in the case of the panic, the logs in the optimized_mir fns wouldn't be output, but the surrounding ones in instance_mir would... which I couldn't wrap my head around

oli (Oct 08 2019 at 12:48, on Zulip):

well

oli (Oct 08 2019 at 12:48, on Zulip):

not quite

oli (Oct 08 2019 at 12:48, on Zulip):

sorry

Paul Faria (Oct 08 2019 at 12:48, on Zulip):

I could find instances of the calls throughout the rest of the log, but not in that specific instance

oli (Oct 08 2019 at 12:48, on Zulip):

so

oli (Oct 08 2019 at 12:48, on Zulip):

this is a query

Paul Faria (Oct 08 2019 at 12:48, on Zulip):

Oh, is it memoized?

Paul Faria (Oct 08 2019 at 12:52, on Zulip):

Oh... I just found https://github.com/rust-lang/rust/blob/9e35a2811d8c65e9473176b8656a3201b7e152c7/src/librustc/query/mod.rs#L123

oli (Oct 08 2019 at 12:53, on Zulip):

That's only part of it

oli (Oct 08 2019 at 12:53, on Zulip):

I can't find the place where the swtich on def_id.is_local() happens and decides on whether a query is run or fetched from the store

oli (Oct 08 2019 at 12:54, on Zulip):

memoization is just a part of it

oli (Oct 08 2019 at 12:54, on Zulip):

if you query something from another crate, its value is loaded from metadata

Paul Faria (Oct 08 2019 at 12:57, on Zulip):

Is that not https://github.com/rust-lang/rust/blob/9e35a2811d8c65e9473176b8656a3201b7e152c7/src/librustc/query/mod.rs#L124 ?

oli (Oct 08 2019 at 13:01, on Zulip):

That decides to cache it, but I'm wondering what code decides where to load it from

Paul Faria (Oct 08 2019 at 13:03, on Zulip):

Following the proc macros I came to this: https://github.com/rust-lang/rust/blob/9e35a2811d8c65e9473176b8656a3201b7e152c7/src/librustc_macros/src/query.rs#L331-L353

Paul Faria (Oct 08 2019 at 13:03, on Zulip):

But having a hard to trying to map all of that in my head between the various invocations (coffee hasn't kicked in yet)

Paul Faria (Oct 08 2019 at 13:03, on Zulip):

I'm not going to have time this morning either. Need to rush to an appointment.

Paul Faria (Oct 08 2019 at 13:04, on Zulip):

Now that I know there was a third fn to look at though, I know I can follow up on this tonight and it will make my head hurt less :)

oli (Oct 08 2019 at 13:07, on Zulip):

heh

oli (Oct 08 2019 at 13:07, on Zulip):

I wouldn't worry about it too much, the query system can be used without understanding

oli (Oct 08 2019 at 13:08, on Zulip):

I understood it mostly half a year ago, but it got refactored a lot

oli (Oct 08 2019 at 13:08, on Zulip):

since I'm not doing anything in it I don't expect to get back to understanding any time soon

Paul Faria (Oct 08 2019 at 14:04, on Zulip):

Crazy thought on the way to my appointment: what if instead of storing the cache in Body, it was passed with it? Or if there was another struct that contained both Body and Cache?

Paul Faria (Oct 08 2019 at 14:06, on Zulip):

We could keep the cache mut while the body stays sharable, and that means it could recompute just like before

oli (Oct 08 2019 at 14:14, on Zulip):

well, you can always create an eval_always query that calls optimized_mir and computes the cache

oli (Oct 08 2019 at 14:14, on Zulip):

but that won't work inside the mir pass pipeline

oli (Oct 08 2019 at 14:14, on Zulip):

and it would then be rather hard to enforce clearing of the cache in the pipeline

oli (Oct 08 2019 at 14:14, on Zulip):

maybe it would be ok to just always eagerly compute it in the pipeline?

oli (Oct 08 2019 at 14:14, on Zulip):

is it ever actually used from a cached version?

Paul Faria (Oct 08 2019 at 14:47, on Zulip):

It did panic when it wasn't available for num::wrapping_add

oli (Oct 08 2019 at 15:05, on Zulip):

during compilation of libcore?

Paul Faria (Oct 09 2019 at 00:17, on Zulip):

I'm actually not sure during which crate. I'll have to recreate the scenario

Paul Faria (Oct 09 2019 at 01:58, on Zulip):

Looks like the panics came from compiling compiler_builtins:

1947420:[INFO  rustc::ty] PAUL: Getting Item(DefId(2:13848 ~ core[9364]::num[0]::{{impl}}[8]::wrapping_add[0])) for "compiler_builtins"
1947421:thread 'rustc' panicked at 'Expected cache.predecessors to be `Some(...)` for block at: /home/pfaria/projects/rust/src/libcore/num/mod.rs:1117:13: 1119:14', src/librustc/mir/mod.rs:230:9
1947422-stack backtrace:
1947423-   0: backtrace::backtrace::libunwind::trace
Paul Faria (Oct 09 2019 at 13:01, on Zulip):

@oli so I've been playing with that idea of inverting the relationship: OwnedCache { cache: Cache, body: Body }, and it seems to work pretty well so far (I haven't gotten it completely compiling yet, I'll see if I can wrap that up tonight at the lower levels to make sure the borrowing is agreeable with the compiler). I moved the predecessor methods there as well as the methods on Body that invalidate the cache.

Paul Faria (Oct 10 2019 at 03:26, on Zulip):

So I actually got a good amount compiling at this point. librustc and librustc_data_structures are both compiling, but right now I'm looking at 75 errors from old fn's I removed plus the new changes that need to be accounted for. Could you review the progress I made so far and let me know if anything stands out as this clearly won't work? I also wasn't sure if those wrapper types helped at all, but it did make writing the visitor macro significantly easier. It also helps ensure that Body can't be mutated when BorrowedCache is being passed around, but that the cache can still be populated if needed (removing the need for us to sprinkle ensure_predecessors everywhere.

Paul Faria (Oct 10 2019 at 03:28, on Zulip):

Also, regarding the WithPredecessors::predecessors(&mut self) trait fn, I couldn't find a way to write it with self. There were lots of issues with it wanting to consume the &mut and only being satisfied if the T of &mut T was also Copy, which isn't possible since &mut types can't be Copy.

Paul Faria (Oct 14 2019 at 05:50, on Zulip):

Cleaned up the passthrough nightmare. I finally figured out the lifetime issues. librustc_codegen_ssa should be good at this point. Going to try tackling librustc_mir during the week. Given that I have 181 errors in that crate, I'm expecting it to take a while to get through all of them.

Paul Faria (Oct 19 2019 at 19:12, on Zulip):

Got really busy with personal stuff this week. Rebased recently and I need to redo codegen_ssa again :sob:. Someone built a helper that checks predecessors and now I need to get &mut BodyCache<...> through all the modules. Might have time to look at that later tonight or tomorrow afternoon for a few hours.

Paul Faria (Oct 23 2019 at 02:46, on Zulip):

@oli I've been working through the idea you posted a few days ago on the PR.

Paul Faria (Oct 23 2019 at 02:51, on Zulip):

I had to ensure the BodyCache returned a new type ReadOnlyBodyCache that ensures the cache is computed before it can be created. This is necessary to fulfill the usage within dominators, which requires the Cache and the Body (and it's not possible to implement the traits against tuples). The issue I'm running into is that to create ReadOnlyBodyCache, I need an fn like:

fn read_only(&'b mut self) -> ReadOnlyBodyCache<'b, 'tcx> {
  self.ensure_predecessors();
  ReadOnlyBodyCache {
    cache: &self.cache,
    body: self.body,
  }
}

However, this restricts self from being borrowed immutably elsewhere because the mutable borrow is held. How would I work around this? Would it be alright to make ensure_predecessors public, and generating ReadOnlyBodyCache will just panic if the cache isn't computed?

Paul Faria (Oct 23 2019 at 02:54, on Zulip):

Would it make sense to wrap something like that in a macro to make the ensure_predecessors call pseudo-private?

oli (Oct 23 2019 at 12:40, on Zulip):

you could make it a method that takes self by value. To go back to a mutable version you can create a second method to convert back

Paul Faria (Oct 23 2019 at 14:02, on Zulip):

Oh I hadn't thought about converting back. That should work. Thanks!

Paul Faria (Oct 25 2019 at 14:04, on Zulip):

@oli when you get a chance, I've fixed all the type errors up to librustc_mir/transform. What's the best place for me to get started on allowing the MirPasses to pass along the caches? I'm also wondering what to do about the fact that some will need to take &mut BodyCache<&'a mut Body<'tcx>>, and some will need ReadOnlyBodyCache<'a, 'tcx>.

Paul Faria (Oct 25 2019 at 14:04, on Zulip):

That last bit is regarding the run_pass fn

Paul Faria (Oct 25 2019 at 14:05, on Zulip):

Hmmm maybe passing around a &mut BodyCache<Body<'tcx>>?

Paul Faria (Oct 25 2019 at 14:05, on Zulip):

And then converting that to the other types as needed?

oli (Oct 25 2019 at 14:23, on Zulip):

can't all MirPasses take &mut BodyCache?

Paul Faria (Oct 25 2019 at 14:34, on Zulip):

They could, but then I'm not sure whether to pass around &mut BodyCache<Body<'tcx>> or &mut BodyCache<&'a mut Body<'tcx>>

Paul Faria (Oct 25 2019 at 14:34, on Zulip):

I assume the latter will give me issues with returning &'a Body<'tcx>, right?

oli (Oct 25 2019 at 14:37, on Zulip):

all MirPasses will require &mut BodyCache<&mut Body>, so you'll always need the latter

oli (Oct 25 2019 at 14:38, on Zulip):

the passes may not use the mutability

oli (Oct 25 2019 at 14:38, on Zulip):

but that's not a problem

Paul Faria (Oct 25 2019 at 14:48, on Zulip):

Hm, I may need to fix some things then in the visitors. In order to get the lifetimes in agreement I did what you had recommended re: fn read_only(self) -> ReadOnlyBodyCache<...>, but that's not possible with &mut BodyCache<...> since I can't move out of the reference. The non-mut visitors were updated to only work with ReadOnlyBodyCache.

Paul Faria (Oct 25 2019 at 14:49, on Zulip):

I'll review later tonight after work

oli (Oct 25 2019 at 14:59, on Zulip):

btw, I'm going on vacation tomorrow, I won't be online again until the 4th of November

Paul Faria (Oct 25 2019 at 15:01, on Zulip):

Enjoy! We'll have a small overlap then since I'll be on vacation from the 7-10th of November.

vertexclique (Oct 25 2019 at 15:52, on Zulip):

@oli Enjoy! I will update you with the latest changes in intrinsic promotion topic when you come back!

oli (Oct 25 2019 at 15:53, on Zulip):

thx!

Paul Faria (Nov 05 2019 at 00:57, on Zulip):

@oli welcome back! I hope it was a wonderful vacation. In your absence I got https://github.com/rust-lang/rust/pull/64736 fully compiling, passing tests, and reasonably cleaned up. I was wondering if you think the variable renames all over the place were overkill (I can undo them if it makes the code easier to read). I also left a comment on one area I really dislike, but not sure how to clean up. Either way I think this version makes me much more comfortable than the one that relied on unwraps/asserts more often.

oli (Nov 05 2019 at 09:04, on Zulip):

slowly catching up :D

oli (Nov 05 2019 at 09:04, on Zulip):

I have 400 new messages from github and around 20 pings on zulip

Paul Faria (Nov 05 2019 at 13:16, on Zulip):

e_O

Paul Faria (Nov 05 2019 at 13:17, on Zulip):

No rush. I'll help out in the rustc guide for the time being

Paul Faria (Nov 07 2019 at 14:52, on Zulip):

@oli anything else I should look at in my PR? Also, did you ever see my comments in the PR about the mir_built query?

Paul Faria (Nov 07 2019 at 14:52, on Zulip):

Wanted to see if I could take care of anything before I head out before vacation. I still have a few hours today

oli (Nov 08 2019 at 11:15, on Zulip):

I responded on the PR

oli (Nov 08 2019 at 11:16, on Zulip):

I'm not quite sure why you need the readonly thing. Couldn't &CachedBody work similarly?

Paul Faria (Nov 11 2019 at 13:45, on Zulip):

The difference is that ReadOnlyBodyCache is guaranteed to have a computed cache, while &BodyCache is not

Paul Faria (Nov 11 2019 at 13:46, on Zulip):

Back from vacation. I'll try reviewing this morning, but might not get back to it until late tonight/early tomorrow

oli (Nov 11 2019 at 14:58, on Zulip):

hmm... right, so this is helping in not forgetting to pre-generate the cache?

Paul Faria (Nov 11 2019 at 18:13, on Zulip):

Yes, exactly. I initially thought it was overkill, but it made the cleanup work much easier once I got into it.

oli (Nov 11 2019 at 18:17, on Zulip):

ok seems fine then. I still think we can do without cloning though :slight_smile:

Paul Faria (Nov 12 2019 at 14:19, on Zulip):

You're referring to the clone in unsafety_check_result? So I can fix that by ensuring the cache is computed at the end of mir_built, but this will mean that cache is invalidated once it's stolen in mir_const ( https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/mod.rs#L188 )

oli (Nov 12 2019 at 14:38, on Zulip):

why does stealing invalidate the cache?

Paul Faria (Nov 12 2019 at 16:31, on Zulip):

Stealing doesn't, but the passes running on it can modify the body, which would invalidate the cache, right?

Paul Faria (Nov 12 2019 at 16:32, on Zulip):

Sorry, I was referring to what was happening inside of mir_const and not that stealing itself would invalidate the cache

Paul Faria (Nov 12 2019 at 16:32, on Zulip):

Particularly the run_passes portion of that fn

Paul Faria (Nov 14 2019 at 03:04, on Zulip):

@oli I went ahead and implemented your suggestions yesterday. Is there anything other major item? Anything I could improve as well, e.g. comments anywhere within the code, or something else along those lines?

oli (Nov 14 2019 at 12:31, on Zulip):

I'll do a round of reviews. I think some documentation explaining things that aren't immediately obvious from readign the code would be good indeed. I'll leave comments wherever I see such situations

Last update: Nov 17 2019 at 08:25UTC