Stream: t-compiler/wg-mir-opt

Topic: #66339 const prop causes ICE due to bad AllocId


Wesley Wiser (Nov 21 2019 at 12:37, on Zulip):

I'm looking into #66339 and I believe the issue is that we lost the code here which handles interning allocated memory. I think I can call intern_const_alloc_recursive() from the right places in ConstProp but it takes a CompileTimeEvalContext which is a InterpCx<'_, '_, CompileTimeInterpreter> and ConstProp has a InterpCx<'_, '_, ConstPropMachine>.

I think it's possible to make this code more generic over the specific machine so it can be called from ConstProp but before I started doing that, I wanted to make sure that seems like the right thing to do. Thoughts?

oli (Nov 21 2019 at 12:40, on Zulip):

hmm... I wonder why it's hardcoded to CompileTimeInterpreter

oli (Nov 21 2019 at 12:40, on Zulip):

are we using anything that only exists for that?

oli (Nov 21 2019 at 12:40, on Zulip):

if not, yea, just make it generic

Wesley Wiser (Nov 21 2019 at 12:41, on Zulip):

It relies on some of the associated types being specific types like ()

Wesley Wiser (Nov 21 2019 at 12:46, on Zulip):

Actually, I could be wrong. I tried this a while ago and ran into issues with having to add nasty type signatures everywhere to make the code generic but not I can't find where we actually need those associated types to be specific things.

oli (Nov 21 2019 at 13:31, on Zulip):

oh, yea these exist solely for miri's stacked borrows logic

oli (Nov 21 2019 at 13:31, on Zulip):

just keep them at () everywhere

Wesley Wiser (Nov 21 2019 at 13:40, on Zulip):

I can't define a polymorphic type alias right? Having this would make the diff so much neater:

type CompileTimeInterpCx<'tcx, 'mir, M: Machine<'tcx, 'mir, AllocExtra = (), PointerTag = ()>> = InterpCx<'tcx, 'mir, M>;
Wesley Wiser (Nov 21 2019 at 13:41, on Zulip):

There's a lot of this kind of thing in the diff:

-struct InternVisitor<'rt, 'mir, 'tcx> {
+struct InternVisitor<'rt, 'mir, 'tcx, M>
+where
+    M: Machine<
+        'mir,
+        'tcx,
+        MemoryKinds = !,
+        PointerTag = (),
+        ExtraFnVal = !,
+        FrameExtra = (),
+        MemoryExtra = (),
+        AllocExtra = (),
+        MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>,
+    >,
+{
oli (Nov 21 2019 at 13:42, on Zulip):

you can, but the bounds are somewhat unchecked: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=91fbdf57bcb584898d9025fb70690a63

oli (Nov 21 2019 at 13:42, on Zulip):

not sure if they'll work for the use case

Wesley Wiser (Nov 21 2019 at 13:44, on Zulip):

I feel like I've tried this in the past and it didn't work because it ignored the bounds or something

Wesley Wiser (Nov 21 2019 at 13:44, on Zulip):

Thanks though

RalfJ (Nov 22 2019 at 14:43, on Zulip):

isn't it bad if const-prop interns stuff?

RalfJ (Nov 22 2019 at 14:44, on Zulip):

IMO that should never ever happen

RalfJ (Nov 22 2019 at 14:44, on Zulip):

interning is for the results consts and statics and the like

Wesley Wiser (Nov 22 2019 at 14:47, on Zulip):

hm iirc const prop prior to the "use miri" changes interned things in some situations as well

Wesley Wiser (Nov 22 2019 at 14:48, on Zulip):

when we moved to miri, that broke and caused issues like #66339

Wesley Wiser (Nov 22 2019 at 14:48, on Zulip):

perhaps that was the wrong behavior all along?

Wesley Wiser (Nov 22 2019 at 14:48, on Zulip):

Are you saying it should never intern anything? Or just certain things?

oli (Nov 22 2019 at 15:05, on Zulip):

if we produce new large constant values (e.g. an array), we need to store them somewhere, thus we intern them and provide a ByRef to them

RalfJ (Nov 22 2019 at 15:14, on Zulip):

okay... hm.
let me rephrase: interning should only happen "at the end", when a certain value was computed for something and is being inserted into the MIR. that basically adds a new static and puts the result of const-prop into that static. but intermediate computations shouldn't be interned.

oli (Nov 22 2019 at 15:15, on Zulip):

right

Last update: Dec 12 2019 at 00:50UTC