Stream: t-compiler/wg-mir-opt

Topic: Using `InterpCx` more


Wesley Wiser (Jun 12 2019 at 13:50, on Zulip):

Hi @oli

This is the discussion thread for #61769.

Wesley Wiser (Jun 12 2019 at 13:51, on Zulip):

I'm feeling like we should just close the PR and work on the refactoring to use InterpCx more.

oli (Jun 12 2019 at 13:51, on Zulip):

sorry, I should have pushed for this stronger to begin with

Wesley Wiser (Jun 12 2019 at 13:51, on Zulip):

What do you think?

Wesley Wiser (Jun 12 2019 at 13:51, on Zulip):

No worries! I forgot about that as well.

oli (Jun 12 2019 at 13:52, on Zulip):

Either way is fine with me. But if you're willing to work on this, I think in sumtotal it will be less work if you drop the PR, but it will take longer to get somewhere

Wesley Wiser (Jun 12 2019 at 13:53, on Zulip):

I'm definitely interested in working on that. Are there specific steps you can think of that need to happen, or is it just one large change?

oli (Jun 12 2019 at 13:54, on Zulip):

you can begin with just getting rid of our own OpTy storage and reusing the one from the lowermost frame.

oli (Jun 12 2019 at 13:54, on Zulip):

this is zero change in behaviour and just a change in where we store stuff

Wesley Wiser (Jun 12 2019 at 13:54, on Zulip):

Ok

oli (Jun 12 2019 at 13:54, on Zulip):

as an interim step you may even want to stop accessing our storage directly and add setter and getters

oli (Jun 12 2019 at 13:55, on Zulip):

then, in the storage-reuse step you can just change the setters and getters without impacting a lot of code

oli (Jun 12 2019 at 13:55, on Zulip):

this can be in one PR and just two commits after each other

Wesley Wiser (Jun 12 2019 at 13:55, on Zulip):

Ok

Wesley Wiser (Jun 12 2019 at 13:55, on Zulip):

"lowermost" you mean the base stack frame or the stack fame currently being evaluated?

oli (Jun 12 2019 at 13:56, on Zulip):

the base stack frame, we should only ever have one when inside the const propagator

Wesley Wiser (Jun 12 2019 at 13:56, on Zulip):

Ok

oli (Jun 12 2019 at 13:56, on Zulip):

you only ever get more stack frames once we start evaluating function calls

oli (Jun 12 2019 at 13:56, on Zulip):

and even then they should be finished once control comes back to you

oli (Jun 12 2019 at 13:56, on Zulip):

hmm... although in the presence of errors that may not be true

oli (Jun 12 2019 at 13:57, on Zulip):

but we'll cross that bridge once we get to function call propagation

Wesley Wiser (Jun 12 2019 at 13:57, on Zulip):

We'll probably need to "unwind" the stack back to the callee

Wesley Wiser (Jun 12 2019 at 13:57, on Zulip):

Sure

oli (Jun 12 2019 at 13:57, on Zulip):

ugh, nah, we'll just not prop the call in case of errors :D

oli (Jun 12 2019 at 13:57, on Zulip):

miri doesn't even support unwinding yet

Wesley Wiser (Jun 12 2019 at 13:57, on Zulip):

Heh, sure :)

Wesley Wiser (Jun 12 2019 at 13:58, on Zulip):

Then to actually use InterpCx, I guess I need to set up that frame?

Wesley Wiser (Jun 12 2019 at 13:58, on Zulip):

Or do we already have one?

oli (Jun 12 2019 at 14:00, on Zulip):

we have one

oli (Jun 12 2019 at 14:01, on Zulip):

but we specifically don't create locals iirc

Wesley Wiser (Jun 12 2019 at 14:01, on Zulip):

Ok. I can dig into that more tonight

oli (Jun 12 2019 at 14:03, on Zulip):

basically we're calling https://github.com/rust-lang/rust/blob/1cbd8a4d686d1411105f26cddf876c5994e69593/src/librustc_mir/const_eval.rs#L46 which in turn creates an empty InterpCx: https://github.com/rust-lang/rust/blob/1cbd8a4d686d1411105f26cddf876c5994e69593/src/librustc_mir/interpret/eval_context.rs#L213

Wesley Wiser (Jun 12 2019 at 14:05, on Zulip):

That's helpful, thanks!

oli (Jun 12 2019 at 14:05, on Zulip):

I think you should be able to just call push_stack_frame after the call to mk_eval_cx

oli (Jun 12 2019 at 14:05, on Zulip):

all information should be available

oli (Jun 12 2019 at 14:05, on Zulip):

although not so sure about the mir::Body

oli (Jun 12 2019 at 14:05, on Zulip):

maybe just feed in an empty mir::Body

oli (Jun 12 2019 at 14:06, on Zulip):

oh, I tried that before, hmm...

Wesley Wiser (Jun 12 2019 at 14:06, on Zulip):

Don't I need the locals though? https://github.com/rust-lang/rust/blob/3d7a1c9dc81b0da3140fe008a2276d6f2c266f10/src/librustc_mir/interpret/eval_context.rs#L504

oli (Jun 12 2019 at 14:06, on Zulip):

ah you are already ripping out the LocalDecls from the mir::Body

Wesley Wiser (Jun 12 2019 at 14:07, on Zulip):

Oh, right

oli (Jun 12 2019 at 14:07, on Zulip):

yea :laughing: you're right, we need them, so no empty mir::Body

oli (Jun 12 2019 at 14:07, on Zulip):

but maybe we can create a mostly dummy mir::Body with just the ripped out info

oli (Jun 12 2019 at 14:08, on Zulip):

instead of storing the info in the const propagator, we store it in the mir::Body that we give the first stack frame

oli (Jun 12 2019 at 14:08, on Zulip):

OK, I realize it wasn't as straight forward as I thought at the beginning

oli (Jun 12 2019 at 14:08, on Zulip):

once upon a time we had a dummy stack frame

Wesley Wiser (Jun 12 2019 at 14:08, on Zulip):

Still doesn't sound that bad though :)

oli (Jun 12 2019 at 14:08, on Zulip):

yea

Wesley Wiser (Jun 12 2019 at 14:09, on Zulip):

I'll try to work on this a bit tonight and report back if I have any questions

oli (Jun 12 2019 at 14:09, on Zulip):

awesome

Wesley Wiser (Jun 12 2019 at 14:09, on Zulip):

ttyl

Wesley Wiser (Jun 14 2019 at 10:17, on Zulip):

So I've started working on this and I've got some in progress changes but I'm running into InterpretCx::push_stack_frame() needing a &'mir Body while ConstPropagator::visit_body() also needs a &mir Body at the same time. See here

Wesley Wiser (Jun 14 2019 at 10:17, on Zulip):

Any ideas?

oli (Jun 14 2019 at 10:52, on Zulip):

yes, create a dummy mir::Body and put the things you stole from the other mir::Body in there

oli (Jun 14 2019 at 10:53, on Zulip):

not sure what creating a dummy mir::Body entails, but you should be able to use Default::default for many things

Wesley Wiser (Jun 18 2019 at 10:47, on Zulip):

So in order to push_stack_frame(), I need an Instance. As far as I can tell, there's no SubstsRef for the Body we're operating on. I tried this https://github.com/rust-lang/rust/commit/faf730b0c9725b44cd54292e5be1b8c5e056bff0#diff-9e103702275cbef342c2decd3395bf3bR176 but that causes normalization errors because there are unresolved generics. Is identity_for_item() the right thing to use here or is there something else I should be using to get a SubstsRef?

oli (Jun 18 2019 at 10:54, on Zulip):

hmm... I wonder how much we really need the Instance in each stack frame

oli (Jun 18 2019 at 10:54, on Zulip):

mostly for substs I presume, but maybe we sometimes need the DefId, too?

oli (Jun 18 2019 at 10:54, on Zulip):

I think for now you can add a //HACK comment and just create the instance manually instead of via new

Wesley Wiser (Jun 18 2019 at 10:59, on Zulip):

Should I just pass in an empty SubstsRef?

Wesley Wiser (Jun 18 2019 at 11:11, on Zulip):

Oh, fyi the assert is not coming from Instance::new(), it's later when we try to get the layout of a local that isn't fully normalized.

Wesley Wiser (Jun 18 2019 at 11:12, on Zulip):

For example:

error: internal compiler error: src/librustc_traits/normalize_erasing_regions.rs:43: could not fully normalize `<T as iter::traits::collect::IntoIterator>::IntoIter`

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:643:9
stack backtrace:
   0: std::sys_common::backtrace::print
   1: std::panicking::default_hook::{{closure}}
   2: std::panicking::default_hook
   3: rustc::util::common::panic_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::begin_panic
   6: rustc_errors::Handler::bug
   7: rustc::util::bug::opt_span_bug_fmt::{{closure}}
   8: rustc::ty::context::tls::with_opt::{{closure}}
   9: rustc::ty::context::tls::with_context_opt
  10: rustc::ty::context::tls::with_opt
  11: rustc::util::bug::opt_span_bug_fmt
  12: rustc::util::bug::bug_fmt
  13: rustc::ty::context::GlobalCtxt::enter_local
  14: rustc_traits::normalize_erasing_regions::normalize_ty_after_erasing_regions
  15: rustc::ty::query::__query_compute::normalize_ty_after_erasing_regions
  16: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::normalize_ty_after_erasing_regions>::compute
  17: rustc::dep_graph::graph::DepGraph::with_task_impl
  18: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  19: <rustc::traits::query::normalize_erasing_regions::NormalizeAfterErasingRegionsFolder as rustc::ty::fold::TypeFolder>::fold_ty
  20: rustc::traits::query::normalize_erasing_regions::<impl rustc::ty::context::TyCtxt>::normalize_erasing_regions
  21: rustc_mir::interpret::eval_context::InterpretCx<M>::layout_of_local
  22: rustc_mir::interpret::operand::<impl rustc_mir::interpret::eval_context::InterpretCx<M>>::access_local
  23: rustc_mir::transform::const_prop::ConstPropagator::get_const
  24: rustc_mir::transform::const_prop::ConstPropagator::eval_place::{{closure}}
  25: rustc::mir::Place::iterate2
  26: <rustc_mir::transform::const_prop::ConstPropagator as rustc::mir::visit::MutVisitor>::visit_statement
  27: rustc::mir::visit::MutVisitor::visit_body
  28: <rustc_mir::transform::const_prop::ConstProp as rustc_mir::transform::MirPass>::run_pass
oli (Jun 18 2019 at 11:25, on Zulip):

oh

oli (Jun 18 2019 at 11:25, on Zulip):

I thought it was during creation

oli (Jun 18 2019 at 11:26, on Zulip):

@Wesley Wiser maybe we should bail out during layout_of_local if the type passed to it needs_substs() (bail out with err!(TooGeneric)

oli (Jun 18 2019 at 11:30, on Zulip):

oh, no it's not quite that easy

oli (Jun 18 2019 at 11:30, on Zulip):

it should be in monomorphize_with_substs

oli (Jun 18 2019 at 11:31, on Zulip):

basically if substituted.needs_subst()

oli (Jun 18 2019 at 11:31, on Zulip):

before doing the normalization run

oli (Jun 18 2019 at 11:31, on Zulip):

if substitution didn't make it monomorphic, bail out

Wesley Wiser (Jun 18 2019 at 13:14, on Zulip):

Ok, that seems to work although most of the const-prop tests are broken now :sweat:

I'll investigate more tonight.

oli (Jun 18 2019 at 13:19, on Zulip):

you only moved the storage from the custom OpTy storage to the first frame's storage, right?

oli (Jun 18 2019 at 13:19, on Zulip):

no const prop logic has been changed to make (more) use of the InterpCx methods

Wesley Wiser (Jun 18 2019 at 13:22, on Zulip):

Sort of. I'm not 100% sure of my changes

Wesley Wiser (Jun 18 2019 at 13:23, on Zulip):

This assert was failing https://github.com/rust-lang/rust/blob/faf730b0c9725b44cd54292e5be1b8c5e056bff0/src/librustc_mir/transform/const_prop.rs#L773

Wesley Wiser (Jun 18 2019 at 13:23, on Zulip):

Because self.get_const() was already returning the right const value before we'd even set it.

Wesley Wiser (Jun 18 2019 at 13:24, on Zulip):

Which is not what I'd expected at all.

oli (Jun 18 2019 at 13:28, on Zulip):

hmm... that may be due to push_stack_frame initializing all locals that have neither StorageLive nor StorageDead calls

Wesley Wiser (Jun 18 2019 at 13:29, on Zulip):

Interesting...

Wesley Wiser (Jun 18 2019 at 13:29, on Zulip):

That's helpful

Wesley Wiser (Jun 18 2019 at 13:29, on Zulip):

There's a number of places that use self.ecx to perform a more complex operation. Perhaps some of those are also mutating locals in the frame?

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

they shouldn't, I mean they would have failed before, too

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

maybe just start by checking that after the push_stack_frame all locals are Uninitialized. If they aren't then we know the culprit

oli (Jun 18 2019 at 13:32, on Zulip):

if they are, maybe some of the trace logs shed some light

Wesley Wiser (Jun 18 2019 at 13:34, on Zulip):

That's a good idea

Wesley Wiser (Jun 18 2019 at 13:34, on Zulip):

I'll do that tonight

Wesley Wiser (Jun 25 2019 at 10:00, on Zulip):

@oli Ok, so I'm trying to use InterpretCx more and I have a pretty simple change:

diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs
index b5dbaaa2327..a71989f70f1 100644
--- a/src/librustc_mir/transform/const_prop.rs
+++ b/src/librustc_mir/transform/const_prop.rs
@@ -452,8 +452,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
                 self.eval_operand(op, source_info)
             },
             Rvalue::Ref(_, _, ref place) => {
-                let src = self.eval_place(place, source_info)?;
-                let mplace = src.try_as_mplace().ok()?;
+                let mplace = self.use_ecx(source_info, |this| {
+                    let place = this.ecx.eval_place(place)?;
+                    this.ecx.force_allocation(place)
+                })?;
                 Some(ImmTy::from_scalar(mplace.ptr.into(), place_layout).into())
             },
             Rvalue::Repeat(..) |

However, I'm getting an error when bootstrapping:

error[E0391]: cycle detected when processing `f32::<impl at src/libcore/num/f32.rs:149:1: 464:2>::is_normal`
   --> src/libcore/num/f32.rs:246:28
    |
246 |         self.classify() == FpCategory::Normal
    |                            ^^^^^^^^^^^^^^^^^^
    |
note: ...which requires const-evaluating `f32::<impl at src/libcore/num/f32.rs:149:1: 464:2>::is_normal`...
   --> src/libcore/num/f32.rs:245:5
    |
245 |     pub fn is_normal(self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: ...which again requires processing `f32::<impl at src/libcore/num/f32.rs:149:1: 464:2>::is_normal`, completing the cycle

error: aborting due to previous error

which I think is happening because InterpretCx::eval_place calls eval_static_to_mplace which calls const_eval_raw which tries to load the MIR for the function that's already being processed which causes the cycle. Does that sound correct to you?

oli (Jun 25 2019 at 11:10, on Zulip):

Oh, because it's a promoted of the current function

oli (Jun 25 2019 at 11:12, on Zulip):

@Wesley Wiser you could modify load_mir to not hit tcx.optimized_mirif it's a promoted and the DefId of is the same as any frame's Instance's DefId and just fetch the mir::Body of the promoted from there

oli (Jun 25 2019 at 11:13, on Zulip):

that means you'll also have to steal the promotedfield

Wesley Wiser (Jun 25 2019 at 12:17, on Zulip):

@oli Hmm... that doesn't seem to work because in InterpretCx::load_mir(), our self is not the same InterpretCx that we are using for const prop. There's a new one created here https://github.com/rust-lang/rust/blob/10deeae3263301f1d337721ed55c14637b70c3c7/src/librustc_mir/const_eval.rs#L636

oli (Jun 25 2019 at 13:08, on Zulip):

oh no XD

oli (Jun 25 2019 at 13:08, on Zulip):

hmm... how did this work before?

oli (Jun 25 2019 at 13:08, on Zulip):

oh we had a special eval_place in the const propagator

oli (Jun 25 2019 at 13:09, on Zulip):

hmm... sorry I gotta run, not sure what to do there for promoteds

Wesley Wiser (Jun 25 2019 at 13:11, on Zulip):

Ok, see ya :wave:

Wesley Wiser (Jun 26 2019 at 12:07, on Zulip):

@oli I have some changes I'm playing around with that I think will make this work but I'm not sure if this is the right direction

Wesley Wiser (Jun 26 2019 at 12:07, on Zulip):

Or if you'll like them :laughter_tears:

Wesley Wiser (Jun 26 2019 at 12:08, on Zulip):

https://github.com/rust-lang/rust/compare/master...wesleywiser:ecx_refactoring?expand=1

oli (Jun 26 2019 at 13:12, on Zulip):

@Wesley Wiser heh, right direction, but removing the const_eval_raw unconditionally is not an option

oli (Jun 26 2019 at 13:13, on Zulip):

are you planning on creating a ConstProp machine?

oli (Jun 26 2019 at 13:13, on Zulip):

or what's the intern_alloc change for?

oli (Jun 26 2019 at 13:14, on Zulip):

if so, you could create an eval_promoted machine hook

Wesley Wiser (Jun 26 2019 at 13:30, on Zulip):

@oli the intern_alloc change is because InterpretCx::eval_static_to_mplace() is generic over the Machine but eval_body_using_ecx() was hardcoded to CompileTimeEvalContext. So in order to call it from eval_static_to_mplace(), I had to make it generic as well.

oli (Jun 26 2019 at 13:30, on Zulip):

ah XD

Wesley Wiser (Jun 26 2019 at 13:30, on Zulip):

eval_promoted sounds like a better name for the trait method to me

oli (Jun 26 2019 at 13:30, on Zulip):

and there I was hoping for a const prop machine :P

oli (Jun 26 2019 at 13:31, on Zulip):

so anyway, I'm not quite sure how to do this best

Wesley Wiser (Jun 26 2019 at 13:31, on Zulip):

heh maybe that's the right answer but I don't think I have a good enough grasp of the miri architecture to do that correctly

oli (Jun 26 2019 at 13:31, on Zulip):

We don't want to duplicate computing promoteds

Wesley Wiser (Jun 26 2019 at 13:32, on Zulip):

Duplicate between what?

Wesley Wiser (Jun 26 2019 at 13:32, on Zulip):

Oh, the other code in ConstProp?

oli (Jun 26 2019 at 13:32, on Zulip):

well, const prop computes the promoted, and then later const eval or codegen compute it again

oli (Jun 26 2019 at 13:32, on Zulip):

because it wasn't cached this time around

oli (Jun 26 2019 at 13:32, on Zulip):

oh

oli (Jun 26 2019 at 13:32, on Zulip):

we are already doing that

Wesley Wiser (Jun 26 2019 at 13:33, on Zulip):

The code in ConstProp will go away once this works

Wesley Wiser (Jun 26 2019 at 13:33, on Zulip):

(I think)

oli (Jun 26 2019 at 13:33, on Zulip):

no I mean, the ConstProp code is ungreat right now

Wesley Wiser (Jun 26 2019 at 13:33, on Zulip):

:+1: very true

oli (Jun 26 2019 at 13:33, on Zulip):

you didn't make it worse by moving to using InterpCx

oli (Jun 26 2019 at 13:34, on Zulip):

either implementation computed (or tried to compute) the promoted

oli (Jun 26 2019 at 13:34, on Zulip):

and then later we compute it again in monomorphize::collect

oli (Jun 26 2019 at 13:34, on Zulip):

so if we have a complex generic promoted, that can be a lot of computation time wasted

Wesley Wiser (Jun 26 2019 at 13:35, on Zulip):

Should we make a query for computing promoted values so they get cached?

oli (Jun 26 2019 at 13:36, on Zulip):

lol, like use the mir::Body of the promoted as the query key?

oli (Jun 26 2019 at 13:37, on Zulip):

While that would cache it, the hashing may be very expensive

Wesley Wiser (Jun 26 2019 at 13:37, on Zulip):

Couldn't we use the index of the promoted in it's parent's Body.promoted?

oli (Jun 26 2019 at 13:38, on Zulip):

but that would then cause the cycle again

Wesley Wiser (Jun 26 2019 at 13:38, on Zulip):

Ah right

Wesley Wiser (Jun 26 2019 at 13:38, on Zulip):

Why are promoted stored this way?

oli (Jun 26 2019 at 13:38, on Zulip):

because it seemed easy

Wesley Wiser (Jun 26 2019 at 13:38, on Zulip):

Ah

oli (Jun 26 2019 at 13:39, on Zulip):

like the alternative is to introduce generic static items in the query system

oli (Jun 26 2019 at 13:39, on Zulip):

and generate DefIds for them, which... seems dangerous

Wesley Wiser (Jun 26 2019 at 13:39, on Zulip):

Ok

Wesley Wiser (Jun 26 2019 at 13:40, on Zulip):

Well it sounds like this isn't making anything worse

oli (Jun 26 2019 at 13:40, on Zulip):

I have a moderately stupid idea

Wesley Wiser (Jun 26 2019 at 13:40, on Zulip):

It's just not necessarily better

Wesley Wiser (Jun 26 2019 at 13:40, on Zulip):

It's probably not that stupid :slight_smile:

oli (Jun 26 2019 at 13:43, on Zulip):

we create a const_qualif query whose return type is (Steal<mir::Body>, &'tcx [mir::Body]) and that query first runs all the pre-const-qualif MirPasses, then does const_qualif and returns the mir::Body in a stealable way and the promoteds just plain

oli (Jun 26 2019 at 13:44, on Zulip):

then the optimized_mir query first runs const_qualif, steals the MIR and runs the rest of the MirPasses

oli (Jun 26 2019 at 13:45, on Zulip):

CopyProp then doesn't need to change InterpCx like you did, but can just invoke the existing code. const_eval will use the const_qualif query when it needs promoted mir::Bodys

oli (Jun 26 2019 at 13:45, on Zulip):

so instead of having a promoted field, one obtains the promoted mir::Body by using the const_qualif query

oli (Jun 26 2019 at 13:46, on Zulip):

cc @eddyb I'm planning weird things with promoteds again, sanity check please

Wesley Wiser (Jun 26 2019 at 13:47, on Zulip):

And that will let us remove the duplication?

Wesley Wiser (Jun 26 2019 at 13:47, on Zulip):

Not sure I'm totally following tbh

Wesley Wiser (Jun 26 2019 at 13:48, on Zulip):

(Still drinking my morning coffee)

oli (Jun 26 2019 at 13:49, on Zulip):

yes, because calling const_eval_raw is a cached query and thus deduplicates

Wesley Wiser (Jun 26 2019 at 13:49, on Zulip):

Oh oh ok

Wesley Wiser (Jun 26 2019 at 13:49, on Zulip):

Sorry, I thought you mean duplicated code

Wesley Wiser (Jun 26 2019 at 13:49, on Zulip):

Not, re evaluating the same promoteds

oli (Jun 26 2019 at 13:49, on Zulip):

nah, that would have been gone via your PRs scheme, too

Wesley Wiser (Jun 26 2019 at 13:49, on Zulip):

Got it

oli (Jun 26 2019 at 13:50, on Zulip):

but let's wait for eddyb's insights

Wesley Wiser (Jun 26 2019 at 13:50, on Zulip):

So the other wrinkle I've found (besides some ICEs I'm still trying to fix) is that eval_static_to_mplace() will also eval statics

oli (Jun 26 2019 at 13:50, on Zulip):

hmm...

Wesley Wiser (Jun 26 2019 at 13:50, on Zulip):

Which we decided the other week we definitely don't want to do in const prop

oli (Jun 26 2019 at 13:51, on Zulip):

we also don't want this to happen inside constants

Wesley Wiser (Jun 26 2019 at 13:51, on Zulip):

Do you have a prefered way to handle that?

oli (Jun 26 2019 at 13:51, on Zulip):

currently that is prevented via const_qualif

oli (Jun 26 2019 at 13:51, on Zulip):

but I'd like InterpCx to prevent that by itself

Wesley Wiser (Jun 26 2019 at 13:51, on Zulip):

I was just going to add a flag or function to the Machine to see if we should do that. But I haven't thought about it very much.

oli (Jun 26 2019 at 13:52, on Zulip):

I think you don't need to do anything fancy, basically the only way you could ever want to eval a static is if there is only one stack frame and that stack frame's instance is also a static item

oli (Jun 26 2019 at 13:52, on Zulip):

my reasoning is as follows:

oli (Jun 26 2019 at 13:53, on Zulip):

if you have a single stack frame and it's not a static, it must be a constant or promoted, and those should not refer to a static

oli (Jun 26 2019 at 13:53, on Zulip):

if you have more stack frames, you are evaluating const fns, and those should also not be evaluating any statics

Wesley Wiser (Jun 26 2019 at 13:54, on Zulip):

Put another way, if the single stack frame is static then whatever requested const evaluation is obviously fine with eval-ing statics?

oli (Jun 26 2019 at 13:54, on Zulip):

yes

Wesley Wiser (Jun 26 2019 at 13:54, on Zulip):

Yeah

oli (Jun 26 2019 at 13:54, on Zulip):

and you can even ICE for non-promoteds

Wesley Wiser (Jun 26 2019 at 13:54, on Zulip):

That makes sense

Wesley Wiser (Jun 26 2019 at 13:54, on Zulip):

Ok, that's really helpful

Wesley Wiser (Jun 26 2019 at 13:54, on Zulip):

Thanks!

oli (Jun 26 2019 at 13:55, on Zulip):

promoteds need to error out some way so that ConstProp can handle it

Wesley Wiser (Jun 26 2019 at 13:55, on Zulip):

Probably just adding a new error variant to the InterpretError enum

oli (Jun 26 2019 at 13:55, on Zulip):

oh actually.... all of these can ICE

oli (Jun 26 2019 at 13:56, on Zulip):

yea just assert!(self.frames.len() == 1 && self.tcx.is_static(self.frame().instance.def_id())); :D

Wesley Wiser (Jun 26 2019 at 13:56, on Zulip):

:thumbs_up:

oli (Jun 26 2019 at 13:56, on Zulip):

or maybe delay_span_bug if that makes anything unhappy

Wesley Wiser (Jun 26 2019 at 13:57, on Zulip):

Ok

eddyb (Jun 26 2019 at 13:57, on Zulip):

you want to do what?!

eddyb (Jun 26 2019 at 13:57, on Zulip):

sorry, just saw the ping :P

oli (Jun 26 2019 at 13:57, on Zulip):

heheh

oli (Jun 26 2019 at 13:58, on Zulip):

there's text above the ping, or do you want another summary?

eddyb (Jun 26 2019 at 13:58, on Zulip):

ah, above, not below?

oli (Jun 26 2019 at 13:58, on Zulip):

yes

eddyb (Jun 26 2019 at 13:58, on Zulip):

ah wait I did read above

eddyb (Jun 26 2019 at 13:59, on Zulip):

so instead of having a promoted field, one obtains the promoted mir::Body by using the const_qualif query

eddyb (Jun 26 2019 at 13:59, on Zulip):

this is very scary?

oli (Jun 26 2019 at 13:59, on Zulip):

yes

oli (Jun 26 2019 at 13:59, on Zulip):

but less annoying than having MirPasses try to const eval promoteds

eddyb (Jun 26 2019 at 13:59, on Zulip):

(it's not a question, idk why I put a "?" at the end)

oli (Jun 26 2019 at 14:00, on Zulip):

I see no way to cache promoted const evals during MirPasses

oli (Jun 26 2019 at 14:00, on Zulip):

other than the proposed way

eddyb (Jun 26 2019 at 14:00, on Zulip):

you could have a promoted_mir query that uses const_qualif and then does what you said

oli (Jun 26 2019 at 14:00, on Zulip):

but that would need to modify the main mir::Body, too

oli (Jun 26 2019 at 14:00, on Zulip):

so we need a query that returns both, I think?

eddyb (Jun 26 2019 at 14:01, on Zulip):

yeah, sure, just, don't stick it in const_qualif literally

oli (Jun 26 2019 at 14:01, on Zulip):

oh right

oli (Jun 26 2019 at 14:01, on Zulip):

yea that was a strawman name

eddyb (Jun 26 2019 at 14:02, on Zulip):

okay so the problem is that promoteds being inside the parent Mir is a hack and you want to keep them separate so you can query the promoteds at any time?

oli (Jun 26 2019 at 14:03, on Zulip):

yes

eddyb (Jun 26 2019 at 14:03, on Zulip):

how do you plan to handle this cross-crate?

oli (Jun 26 2019 at 14:04, on Zulip):

place the promoted_mir query in metadata I guess?

oli (Jun 26 2019 at 14:04, on Zulip):

at least the ones that are still reachable

eddyb (Jun 26 2019 at 14:05, on Zulip):

yeah but... the local-crate version needs to return more things :P

eddyb (Jun 26 2019 at 14:05, on Zulip):

@nikomatsakis might care about this discussion, although he's busy

oli (Jun 26 2019 at 14:06, on Zulip):

how is that different from the optimized_mir query?

eddyb (Jun 26 2019 at 14:06, on Zulip):

hmm?

eddyb (Jun 26 2019 at 14:07, on Zulip):

oh, wait, all you need is a query that takes the GlobalId or w/e from miri

oli (Jun 26 2019 at 14:07, on Zulip):

getting a &'tcx [mir::Body] for the promoteds will be no different from the promoted field we have in mir::Body right now

eddyb (Jun 26 2019 at 14:07, on Zulip):

and dispatches either to optimized_mir or to promoted_mir, in the local crate

oli (Jun 26 2019 at 14:08, on Zulip):

that's what const_eval does right now ;)

eddyb (Jun 26 2019 at 14:08, on Zulip):

@oli you said you need to return two things

oli (Jun 26 2019 at 14:08, on Zulip):

well, promoted_mir needs to emit the mir::Body of the main MIR, too

oli (Jun 26 2019 at 14:09, on Zulip):

because that is modified by promoted_consts

eddyb (Jun 26 2019 at 14:09, on Zulip):

yes, that wouldn't work cross-crate, would it?

oli (Jun 26 2019 at 14:09, on Zulip):

and then that will get stolen by optimized_mir

oli (Jun 26 2019 at 14:09, on Zulip):

not sure how stealing works cross crate

eddyb (Jun 26 2019 at 14:09, on Zulip):

my point is that you wouldn't have anything to put there

eddyb (Jun 26 2019 at 14:10, on Zulip):

since the cross-crate MIR is obtained from optimized_mir

oli (Jun 26 2019 at 14:10, on Zulip):

"put" where?

oli (Jun 26 2019 at 14:11, on Zulip):

I would have assumed promoted_mir to return a Steal that when accessed will panic

eddyb (Jun 26 2019 at 14:11, on Zulip):

in that return value of promoted_mir(non_local_def_id)

oli (Jun 26 2019 at 14:11, on Zulip):

but the &'tcx [mir::Body] of the promoteds would still be there

eddyb (Jun 26 2019 at 14:11, on Zulip):

I would've assumed that would be impossible by design :P

oli (Jun 26 2019 at 14:11, on Zulip):

anyway, we can create another wrapper query

eddyb (Jun 26 2019 at 14:11, on Zulip):

so it might be better if there was a third query, that gives you access to only one body at a time, given a GlobalId, and is implemented differently for local and cross-crate

oli (Jun 26 2019 at 14:12, on Zulip):

so optimized_mir steals the main mir::Body and promoted_mir steals the &'tcx [mir::Body] and just returns it

oli (Jun 26 2019 at 14:12, on Zulip):

and we have a hack_promoted_mir query that returns the tuple

eddyb (Jun 26 2019 at 14:13, on Zulip):

not sure I like that. I'd rather have the GlobalId thing. hmm

eddyb (Jun 26 2019 at 14:13, on Zulip):

what about optimization passes and promoted MIR?

oli (Jun 26 2019 at 14:13, on Zulip):

that doesn't work, as promoteds can be generic

eddyb (Jun 26 2019 at 14:13, on Zulip):

what would happen there

oli (Jun 26 2019 at 14:13, on Zulip):

is promoted MIR optimized?

oli (Jun 26 2019 at 14:13, on Zulip):

I thought it wasn't

eddyb (Jun 26 2019 at 14:13, on Zulip):

today, yeah, all the passes that run on the parent body run on promoteds

oli (Jun 26 2019 at 14:13, on Zulip):

ah

oli (Jun 26 2019 at 14:14, on Zulip):

we should bench that

eddyb (Jun 26 2019 at 14:14, on Zulip):

the pass might choose not to

eddyb (Jun 26 2019 at 14:14, on Zulip):

but the pass runner throws both at every pass

oli (Jun 26 2019 at 14:15, on Zulip):

considering the minimalism of promoteds, we should probably just not do that

Wesley Wiser (Jun 26 2019 at 14:30, on Zulip):

What's the next step I should work on?

oli (Jun 26 2019 at 14:33, on Zulip):

uuuh :laughing: do you want to touch the query changes discussed here?

Wesley Wiser (Jun 26 2019 at 14:35, on Zulip):

Heh I can try

Wesley Wiser (Jun 26 2019 at 14:36, on Zulip):

So we're adding a promoted_mir query that returns a &'tcx [mir::Body]?

oli (Jun 26 2019 at 14:37, on Zulip):

as a first step, you can create a promoted_mir query that takes a DefId and returns a &'tcx [mir::Body] by calling optimized_mir and returning that field

Wesley Wiser (Jun 26 2019 at 14:37, on Zulip):

What's going to happen with Body? Is it still going to have a promoted field?

oli (Jun 26 2019 at 14:37, on Zulip):

for now yes

oli (Jun 26 2019 at 14:38, on Zulip):

you can port promoted accessors except MirPasses to use the new query

Wesley Wiser (Jun 26 2019 at 14:38, on Zulip):

We won't hit query cycles if promoted_mir calls optimized_mir to borrow the promoted field?

oli (Jun 26 2019 at 14:38, on Zulip):

not for the places that currently call optimized_mir and then access the promoted field directly

oli (Jun 26 2019 at 14:39, on Zulip):

promoted_mir is essentially a "getter" at the beginning

Wesley Wiser (Jun 26 2019 at 14:39, on Zulip):

Ok

Wesley Wiser (Jun 26 2019 at 14:40, on Zulip):

Does promoted_mir need to call optimized_mir or just const_qualifed?

Wesley Wiser (Jun 26 2019 at 14:41, on Zulip):

Well, I guess that won't work because it returns a Steal which optimized_mir will have taken...

oli (Jun 26 2019 at 14:41, on Zulip):

jup, keep calling optimized_mir for now

oli (Jun 26 2019 at 14:43, on Zulip):

once there's a promoted_mir_inner query that returns (Steal<&'tcx mir::Body>, Steal<&'tcx [mir::Body]>) then promoted_mir won't have to refer to optimized_mir anymore and we can remove the promoted field

oli (Jun 26 2019 at 14:43, on Zulip):

but for now just lay the groundwork

Wesley Wiser (Jun 26 2019 at 14:49, on Zulip):

Ok got it

Wesley Wiser (Jun 26 2019 at 14:49, on Zulip):

And that should be independent of the changes I showed you

Wesley Wiser (Jun 26 2019 at 14:49, on Zulip):

It's just a refactoring

oli (Jun 26 2019 at 14:49, on Zulip):

yes

Wesley Wiser (Jun 26 2019 at 14:50, on Zulip):

Ok, thanks!

oli (Jun 26 2019 at 14:50, on Zulip):

and once that is done, making ConstProp use InterpCx for promoteds becomes trivial

Wesley Wiser (Jun 26 2019 at 14:51, on Zulip):

Is there somebody in particular I should ping for reviews for the next few weeks or just let highfive do its job?

oli (Jun 26 2019 at 14:53, on Zulip):

use highfive, it'll get reassigned appropriately if the assignment is off. You can ping @rust-lang/wg-mir-opt to ask for someone to pick it up

Wesley Wiser (Jun 26 2019 at 14:54, on Zulip):

Perfect. Thanks!

Wesley Wiser (Jun 26 2019 at 14:55, on Zulip):

Have a good trip/fun on your time off/whatever you're doing :slight_smile:

oli (Jun 26 2019 at 14:56, on Zulip):

:sailboat:

oli (Jun 26 2019 at 14:56, on Zulip):

thanks!

Santiago Pastorino (Jul 15 2019 at 19:46, on Zulip):

@Wesley Wiser :+1: to me to this https://github.com/rust-lang/rust/pull/62322

Santiago Pastorino (Jul 15 2019 at 19:47, on Zulip):

saw at some point that you, @oli and @eddyb were discussing about the idea

Santiago Pastorino (Jul 15 2019 at 19:47, on Zulip):

I guess you agreed to do so?

Wesley Wiser (Jul 15 2019 at 19:49, on Zulip):

@Santiago Pastorino Yeah, that came out of https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/Using.20.60InterpCx.60.20more/near/169035775

oli (Jul 15 2019 at 19:49, on Zulip):

Jup, we want the promoted field to get removed from the mir::Body

Wesley Wiser (Jul 15 2019 at 19:50, on Zulip):

cc @oli I thought there were going to be more changes than there were. Perhaps my expectations were off or maybe I missed something...

Santiago Pastorino (Jul 15 2019 at 19:51, on Zulip):

ohh just saw @oli r+, cool then so I don't have to :)

Wesley Wiser (Jul 15 2019 at 19:51, on Zulip):

There aren't that many instances of *.promoted though so I think it's correct.

oli (Jul 15 2019 at 19:53, on Zulip):

Jup

Wesley Wiser (Jul 15 2019 at 19:54, on Zulip):

I need to page this back into my brain to figure out what the next step is. @oli I'll probably ping you tomorrow if that's ok to sort that out.

oli (Jul 15 2019 at 20:03, on Zulip):

I'll probably be unavailable until 18 CET or sth.

Wesley Wiser (Jul 17 2019 at 16:32, on Zulip):

@oli So, looking back at the previous conversation here, I think the next step is to introduce the promoted_mir_inner query and remove the promoted field from Body. I've started working on that here. My plan is to actually change promote_consts::promote_candidates() to return the IndexVec<Promoted, Body> of collected promoted values instead of mutating Body.promoted. Then in mir_validated, we can access those promoted values and change the mir_validated query to return ('tcx Steal<Body>, 'tcx Steal<IndexVec<Promoted, Body>>). With that change, I don't think we actually need the promoted_mir_inner query and we'll just change promoted_mir to query mir_validated and then run the optimizations on the promotions.

Wesley Wiser (Jul 17 2019 at 16:32, on Zulip):

Am I on the right track?

oli (Jul 17 2019 at 16:51, on Zulip):

Oh, that makes sense, good that we don't need the inner-hack

Wesley Wiser (Jul 17 2019 at 16:52, on Zulip):

Ok, cool!

Wesley Wiser (Jul 17 2019 at 16:52, on Zulip):

I wasn't sure if I was going completely off track :slight_smile:

oli (Jul 17 2019 at 16:58, on Zulip):

Nope, sounds great and the code lgtm

Wesley Wiser (Jul 22 2019 at 14:53, on Zulip):

So, I've been working more on this and I've come across a few things that are making this harder than I initially expected:

1. Many (most?) of the existing MirPasses operate on mir::Body but they have no idea what that body's DefId is. As a result, they can't easily call tcx.promoted_mir(def_id) instead of body.promoted. I think in general this is resolvable by passing the DefId in from the code calling into the pass but I'm not 100% sure. Perhaps the better thing to do would be to add a def_id: DefId property to Body instead of passing it in everywhere?
2. Some MirPasses operate on both a method Body and it's promoted values at the same time. Example. It's not clear to me if we can seperate the processing of promoted values from their parent Body for these passes as I think we will need to do.
3. The MIR inliner actually copies promoted values from inlined methods into their callsite. I guess this is just another variant of (2) but it's throwing me for a loop.
From looking back in this thread, I think the main reason we're trying to do this is to fix some wasted effort on the compiler's part. @oli noted that we're computing promoted values during ConstProp but then those computed values are discarded and then later codegen recomputes the same promoted values when it needs them.

Perhaps there's a different, easier, way to accomplish the same thing?

For example, what if ConstProp actually replaced the promoted Body with something like:

bb0: {
    _0 = const {value};
    return;
}

I would think that would be much more efficient for codegen to process later which should get us most of the same benefits.

cc @oli

oli (Jul 22 2019 at 18:17, on Zulip):

1. seems like you found some solutions. I'm not sure what would be better either, but having a DefId in the Body doesn't seem like the worst idea, we may be able to move some more things out of Body and access them via queries instead

oli (Jul 22 2019 at 18:19, on Zulip):

2. could be fixed by just running all MirPasses on promoteds, too

oli (Jul 22 2019 at 18:19, on Zulip):

and not do that manually in each pass

oli (Jul 22 2019 at 18:19, on Zulip):

oh, 3 sounds interesting

oli (Jul 22 2019 at 18:21, on Zulip):

We could try to evaluate all promoteds, if that doesn't work (due to generics), run const prop (and other passes) on the promoteds

Wesley Wiser (Jul 22 2019 at 18:38, on Zulip):

1. seems like you found some solutions. I'm not sure what would be better either, but having a DefId in the Body doesn't seem like the worst idea, we may be able to move some more things out of Body and access them via queries instead

I guess a different approach would be to give each promoted a unique id and that would be the key of the promoted_mir query.

Wesley Wiser (Jul 22 2019 at 18:38, on Zulip):

That doesn't seem any better to me though

Wesley Wiser (Jul 22 2019 at 18:39, on Zulip):

2. could be fixed by just running all MirPasses on promoteds, too

Yeah, that's what I was thinking.

Wesley Wiser (Jul 22 2019 at 18:40, on Zulip):

oh, 3 sounds interesting

So, I think the real issue with 3 is that promoted_mir(x) will depend on optimized_mir(x) which then depends on promoted_mir(x) again.

Wesley Wiser (Jul 22 2019 at 18:40, on Zulip):

Because inlining can change the promoted values in a body

oli (Jul 22 2019 at 18:42, on Zulip):

why does promoted_mir(x) depend on optimized_mir(x)?

Wesley Wiser (Jul 22 2019 at 18:44, on Zulip):

optimized_mir will (theoretically) run the inliner which can pull promoted mir from an inlined method into the call site. So the promoted_mir(callsite) depends on first running the inliner at callsite.

Wesley Wiser (Jul 22 2019 at 18:44, on Zulip):

Because promoted_mir(callsite) can't change it's value after the first time it's called

oli (Jul 22 2019 at 19:02, on Zulip):

oh

oli (Jul 22 2019 at 19:02, on Zulip):

that query

oli (Jul 22 2019 at 19:02, on Zulip):

I thought 3 would eliminate it?

Wesley Wiser (Jul 22 2019 at 19:04, on Zulip):

I'm not sure what to do about 3

Wesley Wiser (Jul 22 2019 at 19:05, on Zulip):

(There was supposed to be a paragraph break after 3 but it seems to have been removed)

Wesley Wiser (Aug 05 2019 at 02:24, on Zulip):

@oli I've made a fair bit of progress on this I think. I've now gotten rid of the promoted field on mir::Body entirely and the compiler bootstraps without ICEing to stage 1 (after many unsuccessful attempts). I can see there's some UI tests failing related to additional errors being emitted from const eval but I'm not sure what I did that would have changed anything there. https://github.com/rust-lang/rust/compare/master...wesleywiser:remove_promoted_field?expand=1

oli (Aug 05 2019 at 07:16, on Zulip):

sweet.

oli (Aug 05 2019 at 07:16, on Zulip):

I guess we can get rid of this hack now: https://github.com/rust-lang/rust/compare/master...wesleywiser:remove_promoted_field?expand=1#diff-9e103702275cbef342c2decd3395bf3bL398

oli (Aug 05 2019 at 07:23, on Zulip):

no need for an option in https://github.com/rust-lang/rust/compare/master...wesleywiser:remove_promoted_field?expand=1#diff-c2552a106550d05b69d5e07612f0f812R1510 just use an empty IndexVec

oli (Aug 05 2019 at 07:31, on Zulip):

wrt your additional errors. Maybe they're happening because all MirPasses are now run on promoteds (before copy prop sees them)

oli (Aug 05 2019 at 07:32, on Zulip):

about inlining (https://github.com/rust-lang/rust/compare/master...wesleywiser:remove_promoted_field?expand=1#diff-04789deb41c6e9576b3942e6a92e5551R430) I think the only actionable solution is to change the StaticKind::Promoted to also have a field with the DefId

oli (Aug 05 2019 at 07:32, on Zulip):

at which point we can pull the DefIds out of the StaticKInd enum and put it into the surrounding value

Wesley Wiser (Aug 06 2019 at 10:58, on Zulip):

@oli

I guess we can get rid of this hack now

Done

no need for an option in

This is easy. I'll circle back to this later.

wrt your additional errors. Maybe they're happening because all MirPasses are now run on promoteds (before copy prop sees them)

Maybe? I don't really understand why copy prop would have an effect here.

about inlining

I started making these changes. How does this handle polymorphic promoted values? If I have this:

fn make<T>() -> &'static [T] {
    &[] //promoted[0] in  make: [T; 0]
}

fn test<A, B>() {
    let _ = make<B>();
}

then when we inline make into test, promoted[0] will still have the DefId of make but it won't have any Substs. So when codegen (for example) runs, how does it know to use B for T instead of A?

oli (Aug 06 2019 at 11:49, on Zulip):

we can do the same thing that Unevaluated does and add a field for SubstsRef

oli (Aug 06 2019 at 11:50, on Zulip):

these will need to have gotten adjusted for the calling function's substs, so you'd have to substitute substitutions

Wesley Wiser (Aug 06 2019 at 13:41, on Zulip):

these will need to have gotten adjusted for the calling function's substs, so you'd have to substitute substitutions

:laughing:

That makes sense. Thanks!

Wesley Wiser (Aug 13 2019 at 13:45, on Zulip):

these will need to have gotten adjusted for the calling function's substs, so you'd have to substitute substitutions

So I've been working on this but I'm not really sure how to do that and I don't see any functions https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/subst/type.InternalSubsts.html that seem to do that. Any pointers?

Wesley Wiser (Aug 13 2019 at 13:47, on Zulip):

Oh, wait, is that what https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/relate/trait.Relate.html#tymethod.relate is for?

oli (Aug 13 2019 at 13:48, on Zulip):

@Wesley Wiser that's because that method is on a trait: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/subst/trait.Subst.html

oli (Aug 13 2019 at 13:49, on Zulip):

super confusing name combo

Wesley Wiser (Aug 13 2019 at 13:49, on Zulip):

Yeah, it was hidden under the fold

Wesley Wiser (Aug 13 2019 at 13:49, on Zulip):

Is Relate::relate() what I'm looking for?

oli (Aug 13 2019 at 13:49, on Zulip):

no, you can just call subst directly

oli (Aug 13 2019 at 13:50, on Zulip):

That method should be called loads in the inlining code

oli (Aug 13 2019 at 13:50, on Zulip):

or I'm doing something wrong

oli (Aug 13 2019 at 13:50, on Zulip):

lemme check before I'm talking more nonsense

Wesley Wiser (Aug 13 2019 at 13:51, on Zulip):

No I think you're right

Wesley Wiser (Aug 13 2019 at 13:51, on Zulip):

For example https://github.com/rust-lang/rust/blob/60960a260f7b5c695fd0717311d72ce62dd4eb43/src/librustc_mir/transform/inline.rs#L312

oli (Aug 13 2019 at 13:51, on Zulip):

ah perfect

Wesley Wiser (Aug 13 2019 at 13:52, on Zulip):

So I'll need to call it in the inliner as well

oli (Aug 13 2019 at 13:52, on Zulip):

I screwed up substitutions badly just last week and @eddyb had to dig me out of my hole

Wesley Wiser (Aug 13 2019 at 13:52, on Zulip):

And then also in the collector because the inliner is still running on polymorphic MIR right?

oli (Aug 13 2019 at 13:52, on Zulip):

the collector already monomorphizes

oli (Aug 13 2019 at 13:53, on Zulip):

you don't need to change anything there I believe

Wesley Wiser (Aug 13 2019 at 13:53, on Zulip):

Well I think it just uses collect_neighbors()

Wesley Wiser (Aug 13 2019 at 13:53, on Zulip):

Don't I need to move the collection to https://github.com/rust-lang/rust/blob/60960a260f7b5c695fd0717311d72ce62dd4eb43/src/librustc_mir/monomorphize/collector.rs#L674

Wesley Wiser (Aug 13 2019 at 13:53, on Zulip):

?

Wesley Wiser (Aug 13 2019 at 13:54, on Zulip):

Since that's where the correct DefId and SubstsRef will be in scope

oli (Aug 13 2019 at 13:56, on Zulip):

phew, you scared me a bit

oli (Aug 13 2019 at 13:56, on Zulip):

the current code does the right thing because https://github.com/rust-lang/rust/blob/60960a260f7b5c695fd0717311d72ce62dd4eb43/src/librustc_mir/monomorphize/collector.rs#L1238 has the substs with it

Wesley Wiser (Aug 13 2019 at 13:56, on Zulip):

:sweat:

Wesley Wiser (Aug 13 2019 at 13:57, on Zulip):

Yeah, but because we've split out the promoted MIR from Body, you can't just do what collect_neighbors() is doing because your function might reference promoted MIR from an inlined function and that won't be returned by tcx.promoted_mir(your_body_def_id)

oli (Aug 13 2019 at 13:58, on Zulip):

yea lol I just wrote a big text and realized I'm stupid

Wesley Wiser (Aug 13 2019 at 13:58, on Zulip):

Nah, it's fine. I just want to make sure I'm not doing something dumb either

oli (Aug 13 2019 at 13:58, on Zulip):

so... how does your StaticKind type look right now?

oli (Aug 13 2019 at 13:58, on Zulip):

what info does it propagate?

Wesley Wiser (Aug 13 2019 at 13:58, on Zulip):

(Should have pushed my changes this morning)

oli (Aug 13 2019 at 13:59, on Zulip):

There's a DefId of the root owner function of the promoted and the already adjusted Substs, right?

oli (Aug 13 2019 at 14:00, on Zulip):

so you can create an Instance from this information, but you're right, you need to subst the substs with param_substs before you do

Wesley Wiser (Aug 13 2019 at 14:00, on Zulip):

It's roughly

pub struct Static<'tcx> {
    pub ty: Ty<'tcx>,
    pub kind: StaticKind,
    pub def_id: DefId,
}

pub enum StaticKind {
    Promoted(Promoted, SubstsRef),
    Static,
}
oli (Aug 13 2019 at 14:00, on Zulip):

ah yes, perfect

Wesley Wiser (Aug 13 2019 at 14:01, on Zulip):

So I should do the subst() call in the inliner to correct for any substs at the call site and then again in the collector for any final substs

oli (Aug 13 2019 at 14:01, on Zulip):

yes

Wesley Wiser (Aug 13 2019 at 14:02, on Zulip):

Cool

Wesley Wiser (Aug 13 2019 at 14:02, on Zulip):

I think (hopefully) this is almost finished

oli (Aug 13 2019 at 14:02, on Zulip):

sweet

Wesley Wiser (Aug 13 2019 at 14:03, on Zulip):

Thanks for your help!

oli (Aug 13 2019 at 14:03, on Zulip):

I think we kinda reinvented ty::Const { val: ConstValue::Unevaluated(did, substs), ty }, but that's alright, we can merge those later

Wesley Wiser (Aug 13 2019 at 14:04, on Zulip):

Yeah, that looks very similar and I think I saw code in const_eval_raw_provider that does exactly what we want in that case.

oli (Aug 13 2019 at 14:04, on Zulip):

yea :laughing:

oli (Aug 13 2019 at 14:04, on Zulip):

and inlining will also do the right thing automatically I think

oli (Aug 13 2019 at 14:04, on Zulip):

because it already has logic for constants

Wesley Wiser (Aug 13 2019 at 14:04, on Zulip):

I should look at that then.

Wesley Wiser (Aug 13 2019 at 14:05, on Zulip):

Aren't constants always monomorphic though?

oli (Aug 13 2019 at 14:05, on Zulip):

https://github.com/rust-lang/rust/blob/52c6458320d268d0b5a0b92d67f1fe943c7be518/src/librustc_mir/interpret/operand.rs#L515

oli (Aug 13 2019 at 14:05, on Zulip):

not if they are associated constants of arrays

Wesley Wiser (Aug 13 2019 at 14:05, on Zulip):

Your commit hash doesn't seem to exist :slight_smile:

oli (Aug 13 2019 at 14:06, on Zulip):

:frown: grab it on master then

Wesley Wiser (Aug 13 2019 at 14:09, on Zulip):

Ok yeah it does that too https://github.com/rust-lang/rust/blob/60960a260f7b5c695fd0717311d72ce62dd4eb43/src/librustc_mir/interpret/operand.rs#L546-L551

oli (Aug 13 2019 at 14:13, on Zulip):

sorry, my IDE jumped between files

oli (Aug 13 2019 at 14:13, on Zulip):

that link was entirely bogus

oli (Aug 13 2019 at 14:13, on Zulip):

I meant to link into the interner

oli (Aug 13 2019 at 14:16, on Zulip):

@Wesley Wiser here's the right link: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/inline.rs#L121

oli (Aug 13 2019 at 14:16, on Zulip):

so I guess you don't need to subst anything at all

oli (Aug 13 2019 at 14:17, on Zulip):

it's already substituted

oli (Aug 13 2019 at 14:17, on Zulip):

and if you repeat the substitution, you'd probably get an ICE

Wesley Wiser (Aug 13 2019 at 14:17, on Zulip):

Gotcha

Wesley Wiser (Aug 13 2019 at 14:17, on Zulip):

Thanks!

RalfJ (Aug 14 2019 at 11:09, on Zulip):

you've probably already seen them but in case not, these PRs seem relevant for your discussion:
https://github.com/rust-lang/rust/pull/63495
https://github.com/rust-lang/rust/pull/63497

Wesley Wiser (Aug 16 2019 at 12:05, on Zulip):

Hi @oli :wave:

oli (Aug 16 2019 at 12:06, on Zulip):

hi

Wesley Wiser (Aug 16 2019 at 12:07, on Zulip):

I was looking at the SubstFolder per your comment and it doesn't look like it does anything with promoted MIR at all

Wesley Wiser (Aug 16 2019 at 12:07, on Zulip):

https://github.com/rust-lang/rust/blob/5a6d801bf9399004a0f0a19e510d996f4686c093/src/librustc/ty/subst.rs#L451

Wesley Wiser (Aug 16 2019 at 12:08, on Zulip):

Reading through, I don't think it even touches MIR at all

Wesley Wiser (Aug 16 2019 at 12:08, on Zulip):

Am I looking at the right thing?

Wesley Wiser (Aug 16 2019 at 12:09, on Zulip):

I only found one implementation of trait Subst and it's here so I think this is the right code

Wesley Wiser (Aug 16 2019 at 12:09, on Zulip):

https://github.com/rust-lang/rust/blob/5a6d801bf9399004a0f0a19e510d996f4686c093/src/librustc/ty/subst.rs#L420

oli (Aug 16 2019 at 12:11, on Zulip):

@Wesley Wiser https://github.com/rust-lang/rust/blob/1df512fcaeaf17639c5d28a3045814d6f7a7db97/src/librustc/mir/mod.rs#L3231 is wrong probably

Wesley Wiser (Aug 16 2019 at 12:12, on Zulip):

Oh, gotcha. self.fold_with() dispatches via TypeFoldable.

oli (Aug 16 2019 at 12:12, on Zulip):

it probably was always wrong... the type should have been folded at least

oli (Aug 16 2019 at 12:12, on Zulip):

but now we need to fold the substs, too

oli (Aug 16 2019 at 12:12, on Zulip):

same goes for the visit part

Wesley Wiser (Aug 16 2019 at 12:12, on Zulip):

Ok :thumbs_up:

Wesley Wiser (Aug 16 2019 at 12:13, on Zulip):

I think I can probably go from there

Wesley Wiser (Aug 16 2019 at 12:13, on Zulip):

Sorry to bug you

oli (Aug 16 2019 at 12:17, on Zulip):

no worries at all, please do ask!

Wesley Wiser (Aug 26 2019 at 14:44, on Zulip):

Yay #63580 got merged! :tada:

Wesley Wiser (Aug 26 2019 at 14:48, on Zulip):

I'm starting to look into replacing ConstProp::const_prop() with use of InterpCxwhich I think is now unblocked because we fixed the promoted issue.

oli (Aug 26 2019 at 15:00, on Zulip):

We will have the best const propagator on the planet, you should totally hold a talk about const prop at some point.

Wesley Wiser (Aug 26 2019 at 15:00, on Zulip):

That would be fun

oli (Aug 26 2019 at 15:00, on Zulip):

I mean... what other language reports warnings on failing to apply optimizations....

Wesley Wiser (Aug 26 2019 at 15:01, on Zulip):

I'm on the hunt for a talk to propose for a Rust conference next year anyway :laughing:

Wesley Wiser (Aug 26 2019 at 15:01, on Zulip):

I'm really sorry I missed your RustConf talk, it looked super interesting!

oli (Aug 26 2019 at 15:06, on Zulip):

It was loads of fun

Wesley Wiser (Aug 26 2019 at 15:07, on Zulip):

I caught bits & pieces on Twitter. The stuff on compile time UB looked really interesting! Do you post your slides online?

oli (Aug 26 2019 at 15:09, on Zulip):

The talk was recorded

Wesley Wiser (Aug 26 2019 at 15:10, on Zulip):

Oh, sweet

oli (Aug 26 2019 at 15:10, on Zulip):

Will get uploaded and slides will be available for download

Wesley Wiser (Aug 26 2019 at 15:10, on Zulip):

I'm subscribed to the YT channel so I assume it will pop up sooner or later on there

RalfJ (Aug 26 2019 at 17:17, on Zulip):

looking forward to watching that talk as well :)

Wesley Wiser (Sep 04 2019 at 02:16, on Zulip):

I've got a small patch that replaces ConstProp::eval_place() with InterpCx::eval_place_into_op() and it seems to work great. However, there's a few UI tests that had to be changed because a few places where we were already emitting an error (or errors) now emit an additional error. Is that ok or do we have to match the current behavior?

My branch with the test changes is here.

oli (Sep 04 2019 at 05:06, on Zulip):

We need to figure out a way to remove duplicate errors, but I'm not sure how

oli (Sep 04 2019 at 05:08, on Zulip):

The two cases look OK to me. Once we figure out how to fix them it should all work

Wesley Wiser (Sep 04 2019 at 13:11, on Zulip):

Do you think I should switch and focus on trying to find a solution to those errors or just keep going with the const prop changes?

Can we merge the const prop changes with duplicate errors or do they need to be resolved before that can happen?

oli (Sep 04 2019 at 18:53, on Zulip):

Nah, we can merge them. They are not worse than what we have and the code deduplication is so :ok:

oli (Sep 04 2019 at 18:55, on Zulip):

I don't think it needs to be a focus. Cleaning up const prop should make it easier to fix them at some point

Wesley Wiser (Sep 04 2019 at 18:57, on Zulip):

Ok, I'll keep moving forward then.

Wesley Wiser (Sep 04 2019 at 18:57, on Zulip):

Thanks!

Wesley Wiser (Sep 11 2019 at 11:57, on Zulip):

So I've got a series of commits here which I think are almost ready to go, but I'm stuck on the Rvalue::Ref handling. If I apply the last commit, then I get test failures related to generator layout optimization. These are the failing tests:

    [ui] ui/async-await/async-await.rs
    [ui] ui/async-await/async-closure.rs
    [ui] ui/async-await/async-fn-send-uses-nonsend.rs
    [ui] ui/async-await/async-fn-size-moved-locals.rs
    [ui] ui/async-await/async-fn-size.rs
    [ui] ui/async-await/conditional-and-guaranteed-initialization.rs
    [ui] ui/async-await/drop-order/drop-order-for-locals-when-cancelled.rs
    [ui] ui/async-await/drop-order/drop-order-when-cancelled.rs
    [ui] ui/async-await/generics-and-bounds.rs
    [ui] ui/async-await/issue-60709.rs
    [ui] ui/async-await/issue-61793.rs
    [ui] ui/async-await/issue-62658.rs
    [ui] ui/async-await/issues/issue-55809.rs
    [ui] ui/async-await/issues/issue-61986.rs
    [ui] ui/async-await/move-part-await-return-rest-struct.rs
    [ui] ui/async-await/move-part-await-return-rest-tuple.rs
    [ui] ui/drop/dynamic-drop-async.rs
    [ui] ui/generator/control-flow.rs
    [ui] ui/generator/drop-and-replace.rs
    [ui] ui/generator/issue-44197.rs
    [ui] ui/generator/issue-52398.rs
    [ui] ui/generator/issue-57084.rs
    [ui] ui/generator/issue-58888.rs
    [ui] ui/generator/iterator-count.rs
    [ui] ui/generator/nested_generators.rs
    [ui] ui/generator/overlap-locals.rs
    [ui] ui/generator/static-generators.rs
    [ui] ui/generator/yield-in-box.rs
    [ui] ui/proc-macro/invalid-punct-ident-1.rs
    [ui] ui/proc-macro/invalid-punct-ident-2.rs
    [ui] ui/proc-macro/invalid-punct-ident-3.rs
    [ui] ui/unsized-locals/autoderef.rs

and most of them fail with something like this:

error[E0391]: cycle detected when computing layout of `[generator@src/test/ui/generator/control-flow.rs:26:15: 30:6 {fn(std::ops::Range<i32>) -> <std::ops::Range<i32> as std::iter::IntoIterator>::IntoIter {<std::ops::Range<i32> as std::iter::IntoIterator>::into_iter}, i32, std::ops::Range<i32>, ()}]`
  |
note: ...which requires processing `main::{{closure}}#1`...
 --> src/test/ui/generator/control-flow.rs:27:18
  |
27|         for _ in 0..8 {
  |                  ^^^^
  = note: ...which again requires computing layout of `[generator@src/test/ui/generator/control-flow.rs:26:15: 30:6 {fn(std::ops::Range<i32>) -> <std::ops::Range<i32> as std::iter::IntoIterator>::IntoIter {<std::ops::Range<i32> as std::iter::IntoIterator>::into_iter}, i32, std::ops::Range<i32>, ()}]`, completing the cycle
note: cycle used when processing `main`
 --> src/test/ui/generator/control-flow.rs:24:1
  |
24| fn main() {
  | ^^^^^^^^^

thread 'rustc' panicked at 'aborting due to `-Z treat-err-as-bug=1`', src/librustc_errors/lib.rs:540:13
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
   1: backtrace::backtrace::trace_unsynchronized
   2: std::sys_common::backtrace::_print
   3: std::sys_common::backtrace::print
   4: std::panicking::default_hook::{{closure}}
   5: std::panicking::default_hook
   6: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
   7: rustc::util::common::panic_hook
   8: std::panicking::rust_panic_with_hook
   9: std::panicking::begin_panic
  10: rustc_errors::Handler::panic_if_treat_err_as_bug
  11: rustc_errors::Handler::bump_err_count
  12: rustc_errors::Handler::emit_db
  13: rustc_errors::diagnostic_builder::DiagnosticBuilder::emit
  14: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::layout_raw>::handle_cycle_error
  15: rustc::ty::query::plumbing::JobOwner<Q>::try_get::{{closure}}
  16: rustc_data_structures::cold_path
  17: rustc::ty::query::plumbing::JobOwner<Q>::try_get
  18: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  19: rustc::ty::query::TyCtxtAt::layout_raw
             at src/librustc/ty/query/plumbing.rs:1076
  20: <rustc::ty::layout::LayoutCx<rustc::ty::query::TyCtxtAt> as rustc_target::abi::LayoutOf>::layout_of
             at src/librustc/ty/layout.rs:1965
  21: rustc::ty::layout::<impl rustc::ty::query::TyCtxtAt>::layout_of
             at ./src/librustc/ty/layout.rs:2012
  22: <rustc_mir::interpret::eval_context::InterpCx<M> as rustc_target::abi::LayoutOf>::layout_of
             at src/librustc_mir/interpret/eval_context.rs:191
  23: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::ref_to_mplace
             at src/librustc_mir/interpret/place.rs:295
  24: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::deref_operand
             at src/librustc_mir/interpret/place.rs:321
  25: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::place_projection
             at src/librustc_mir/interpret/place.rs:584
  26: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_place::{{closure}}
             at src/librustc_mir/interpret/place.rs:695
  27: rustc::mir::Place::iterate_over::iterate_over2
  28: rustc::mir::Place::iterate_over::iterate_over2
  29: rustc::mir::Place::iterate_over::iterate_over2
  30: rustc::mir::Place::iterate_over::iterate_over2
  31: rustc::mir::Place::iterate_over::iterate_over2
  32: rustc::mir::Place::iterate_over
  33: rustc::mir::Place::iterate
  34: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_place
             at src/librustc_mir/interpret/place.rs:660
  35: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_rvalue_into_place
             at src/librustc_mir/interpret/step.rs:247
  36: rustc_mir::transform::const_prop::ConstPropagator::const_prop::{{closure}}
             at src/librustc_mir/transform/const_prop.rs:319
  37: rustc_mir::transform::const_prop::ConstPropagator::use_ecx
             at src/librustc_mir/transform/const_prop.rs:237
  38: rustc_mir::transform::const_prop::ConstPropagator::const_prop
             at src/librustc_mir/transform/const_prop.rs:318
  39: <rustc_mir::transform::const_prop::ConstPropagator as rustc::mir::visit::MutVisitor>::visit_statement
             at src/librustc_mir/transform/const_prop.rs:569
  40: rustc::mir::visit::MutVisitor::super_basic_block_data
  41: rustc::mir::visit::MutVisitor::visit_basic_block_data
  42: rustc::mir::visit::MutVisitor::super_body
  43: rustc::mir::visit::MutVisitor::visit_body
  44: <rustc_mir::transform::const_prop::ConstProp as rustc_mir::transform::MirPass>::run_pass
             at src/librustc_mir/transform/const_prop.rs:93
  45: rustc_mir::transform::run_passes::{{closure}}
             at src/librustc_mir/transform/mod.rs:173
  46: rustc_mir::transform::run_passes
             at src/librustc_mir/transform/mod.rs:180
  47: rustc_mir::transform::run_optimization_passes
             at src/librustc_mir/transform/mod.rs:228
  48: rustc_mir::transform::optimized_mir
             at src/librustc_mir/transform/mod.rs:300
  49: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::optimized_mir>::compute
  50: rustc::dep_graph::graph::DepGraph::with_task_impl
  51: rustc::dep_graph::graph::DepGraph::with_task
  52: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::force_query_with_job::{{closure}}::{{closure}}
  53: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::start_query::{{closure}}::{{closure}}
  54: rustc::ty::context::tls::enter_context::{{closure}}
  55: rustc::ty::context::tls::set_tlv
  56: rustc::ty::context::tls::enter_context
  57: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::start_query::{{closure}}

@oli any idea what I should do with this cycle?

Wesley Wiser (Sep 11 2019 at 12:06, on Zulip):

I tried pushing the place_layout we have from ConstProp::const_prop() further down and providing it to InterpCx::eval_rvalue_into_place() but that didn't pan out because the Place containing the generator has a series of projections and layout_of is getting called for one of the intermediate projections so the place_layout we have only applies to the final projection.

oli (Sep 11 2019 at 12:06, on Zulip):

So I'm assuming this optimized_mir invocation happens because of a layout_of invocation

oli (Sep 11 2019 at 12:06, on Zulip):

do you have the query stack available?

Wesley Wiser (Sep 11 2019 at 12:07, on Zulip):

Oh yeah, sorry. It got cutt off when I pasted the stack trace

Wesley Wiser (Sep 11 2019 at 12:07, on Zulip):
query stack during panic:
#0 [optimized_mir] processing `main::{{closure}}#1`
#1 [layout_raw] computing layout of `[generator@src/test/ui/generator/control-flow.rs:26:15: 30:6 {fn(std::ops::Range<i32>) -> <std::ops::Range<i32> as std::iter::IntoIterator>::IntoIter {<std::ops::Range<i32> as std::iter::IntoIterator>::into_iter}, i32, std::ops::Range<i32>, ()}]`
#2 [optimized_mir] processing `main`
#3 [collect_and_partition_mono_items] collect_and_partition_mono_items
end of query stack
oli (Sep 11 2019 at 12:07, on Zulip):

though I wonder why generators would be more special compared with plain closures

oli (Sep 11 2019 at 12:08, on Zulip):

ah

oli (Sep 11 2019 at 12:08, on Zulip):

local vars

oli (Sep 11 2019 at 12:08, on Zulip):

nevermind

oli (Sep 11 2019 at 12:08, on Zulip):

ok, so generators have local variables in their generated struct

oli (Sep 11 2019 at 12:08, on Zulip):

while closures use the stack

oli (Sep 11 2019 at 12:08, on Zulip):

this is because generators need to remember a bunch of their locals across yield points

oli (Sep 11 2019 at 12:09, on Zulip):

so during the computation of the layout of the generator we need to know the MIR in order to know which variables we need

oli (Sep 11 2019 at 12:09, on Zulip):

now the big question is why are we computing the layout of the generator

Wesley Wiser (Sep 11 2019 at 12:10, on Zulip):

So it looks like the line that's failing (at least for the src/test/ui/generator/control-flow.rs test, is this:

(((*(_1.0: &mut [generator@src/test/ui/generator/control-flow.rs:26:15: 30:6 {fn(std::ops::Range<i32>) -> <std::ops::Range<i32> as std::iter::IntoIterator>::IntoIter {<std::ops::Range<i32> as std::iter::IntoIterator>::into_iter}, i32, std::ops::Range<i32>, ()}])) as variant#3).0: std::ops::Range<i32>) = move _2
oli (Sep 11 2019 at 12:11, on Zulip):

:face_palm:

oli (Sep 11 2019 at 12:11, on Zulip):

ok

Wesley Wiser (Sep 11 2019 at 12:11, on Zulip):

So I assume the issue has something to do with the combination of the deref and the tuple projections

oli (Sep 11 2019 at 12:12, on Zulip):

well, it has to do with projecting into the generator from the generator's body

Wesley Wiser (Sep 11 2019 at 12:12, on Zulip):

I'm guessing we didn't need the layout of the bare generator type before since the deref is nested in other projections

oli (Sep 11 2019 at 12:12, on Zulip):

I wonder why we already have a desugared generator at this point

oli (Sep 11 2019 at 12:16, on Zulip):

cc @tmandry we have a small dilemma wrt running const propagation on generator MIR

Wesley Wiser (Sep 11 2019 at 12:17, on Zulip):

Here's the MIR from right before const_prop runs: https://gist.github.com/wesleywiser/46759330b21aac1347641dc0322561ce

oli (Sep 11 2019 at 12:18, on Zulip):

apparently the layout of a generator depends on the MIR after optimizations, but in order to run const propagation we essentially const eval the MIR, which creates constant memory that matches the local's layout

oli (Sep 11 2019 at 12:19, on Zulip):

@Wesley Wiser in the short term I suggest you just skip const prop on the MIR of generators

Wesley Wiser (Sep 11 2019 at 12:20, on Zulip):

Ok :thumbs_up:

oli (Sep 11 2019 at 12:20, on Zulip):

leave a FIXME there, but we should definitely keep talking about it

Wesley Wiser (Sep 11 2019 at 12:20, on Zulip):

Will do

Wesley Wiser (Sep 11 2019 at 12:20, on Zulip):

Thanks!

tmandry (Sep 11 2019 at 20:32, on Zulip):

I think we could get around this by inserting an intermediate query that goes before optimized_mir

tmandry (Sep 11 2019 at 20:33, on Zulip):

and depend on that from the layout code instead

pnkfelix (Sep 12 2019 at 08:19, on Zulip):

"all problems in CS can be solved by another level of indirection"

oli (Sep 12 2019 at 08:43, on Zulip):

we have such a query, the problem is that optimized_mir steals its result to reduce memory load

oli (Sep 12 2019 at 08:43, on Zulip):

so if anyone calls optimized_mir, calling mir_built will fail

oli (Sep 12 2019 at 08:44, on Zulip):

there's no way to resolve cycle errors. You can only break cycles

oli (Sep 12 2019 at 08:46, on Zulip):

I mean we could pessimize generators by skipping const prop on places to a generator struct

oli (Sep 12 2019 at 08:47, on Zulip):

it's probably the best case scenario anyway. If we want real const prop on generators, we need to run const prop on the MIR of the generator that still has all the generator yield things in place

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

though that may screw up const qualif or borrowck

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

aargml

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

ok

oli (Sep 12 2019 at 08:49, on Zulip):

I wonder if we can do something similar to what @Wesley Wiser did for promoteds

oli (Sep 12 2019 at 08:49, on Zulip):

so we get access to the generator state variant types without having to access the MIR

oli (Sep 12 2019 at 08:49, on Zulip):

oh yea, that could work

oli (Sep 12 2019 at 08:51, on Zulip):

I wonder if we could optimize these generator variant types more by eliminating unneeded fields

tmandry (Sep 15 2019 at 03:19, on Zulip):

If you can, that’d be extremely nice

tmandry (Sep 15 2019 at 03:20, on Zulip):

I’m not sure what solution is being proposed at this point though :)

oli (Sep 16 2019 at 10:55, on Zulip):

it's twofold

oli (Sep 16 2019 at 10:56, on Zulip):

one fixes our cycle problem by separating the generator types from the MIR once computed

oli (Sep 16 2019 at 10:56, on Zulip):

the other part is to optimize generator MIR before doing the state machine generation

Wesley Wiser (Sep 17 2019 at 09:39, on Zulip):

@oli So based on the review feedback, I should add hooks for:

1. Accesing a local?
2. Accessing statics
and then create and impl a machine for ConstProp?

oli (Sep 17 2019 at 09:39, on Zulip):

we already have a hook for accessing statics I think

oli (Sep 17 2019 at 09:40, on Zulip):

ah no, that's just for foreign statics

oli (Sep 17 2019 at 09:40, on Zulip):

so yea, a new hook would be good

Wesley Wiser (Sep 17 2019 at 09:40, on Zulip):

Ok

Wesley Wiser (Sep 17 2019 at 09:40, on Zulip):

:thumbs_up:

oli (Sep 17 2019 at 09:41, on Zulip):

this is also great because we can just override enforce_validity instead of reimplementing the checks ourselves

Wesley Wiser (Sep 17 2019 at 09:42, on Zulip):

Oh, for that bit of code left in const_prop() before we call eval_rvalue_into_place()?

oli (Sep 17 2019 at 09:42, on Zulip):

hehe yes

Wesley Wiser (Sep 17 2019 at 09:43, on Zulip):

Yeah, that's great

oli (Sep 17 2019 at 09:43, on Zulip):

oh and you may be able to nix some of the special binary op handling by implementing the binary op hook

oli (Sep 17 2019 at 09:43, on Zulip):

all the function, frame and function hooks can just error for now I guess

Wesley Wiser (Sep 17 2019 at 09:43, on Zulip):

I'll look into that

Wesley Wiser (Sep 17 2019 at 09:44, on Zulip):

What did you think of this? https://github.com/rust-lang/rust/pull/64419#discussion_r325008824

oli (Sep 17 2019 at 09:44, on Zulip):

not necessary for the same PR wrt binary op

Wesley Wiser (Sep 17 2019 at 09:44, on Zulip):

I don't see any reason to restrict access to non-mutating statics

Wesley Wiser (Sep 17 2019 at 09:44, on Zulip):

I could be missing something though

oli (Sep 17 2019 at 09:51, on Zulip):

I don't see any reason either

Wesley Wiser (Sep 17 2019 at 10:19, on Zulip):

If we create a ConstPropMachine, doesn't that mean we won't be able to share const_eval results with the rest of rustc since we'll need to evaluate constants with our machine to get the correct checking?

Wesley Wiser (Sep 17 2019 at 10:33, on Zulip):

Actually, I think this will work out the same as it does currently.

oli (Sep 17 2019 at 11:58, on Zulip):

yea, the const_eval query still invokes the regular machine

Wesley Wiser (Sep 17 2019 at 13:14, on Zulip):

Is there a good way to reuse most of the existing CompileTimeInterpreter machine implementation? I thought maybe I could just compose a new Machine on top of it, do the validation we're interested in, and then dispatch to the underlying CompileTimeInterpreter but the InterpCx is what calls into the Machine so I don't think that will work.

oli (Sep 17 2019 at 13:15, on Zulip):

yea that won't work. What logic do you want to re-use?

oli (Sep 17 2019 at 13:15, on Zulip):

I mean what you can always do is move the impl into a free function with the appropriate generics and call that

Wesley Wiser (Sep 17 2019 at 13:20, on Zulip):

I thought I'd want pretty much all of the implementation but maybe that's the wrong assumption

https://github.com/rust-lang/rust/blob/a44881d892fb4f4a8ed93f8f392bab942fac7a41/src/librustc_mir/const_eval.rs#L302-L479

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

I mean we don't have function calls yet, so all the stack pushing and function resolution stuff can be stubbed out

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

a bunch of stuff is stubbed out in const eval and can just be duplicated or moved onto the trait

Wesley Wiser (Sep 17 2019 at 13:28, on Zulip):

Ah, ok. I won't worry about de-duplicating anything then until I have it actually working.

oli (Sep 17 2019 at 13:28, on Zulip):

yea, the stubs are super tiny

Wesley Wiser (Sep 18 2019 at 01:30, on Zulip):

This might be a dumb question, but do we really need a ConstPropMachine? Couldn't we just add those hooks and put the appropriate implementations in the existing CompileTimeInterpreter Machine?

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

Some things are dead code in const eval and only reachable from const prop

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

I feel like it makes sense to keep them separate, since having some things be ICEs in const eval and errors in const prop will help us catch bugs

Last update: Nov 17 2019 at 08:20UTC