Stream: t-compiler/wg-mir-opt

Topic: [const-prop] Handle remaining MIR Rvalue cases #64890


Wesley Wiser (Oct 11 2019 at 13:14, on Zulip):

Hey @RalfJ when you have time, I think it may be more productive for us to have a conversation here rather than on various threads in my PR :slight_smile:

Wesley Wiser (Oct 11 2019 at 13:22, on Zulip):

The question is, what's the MIR? If in one case the MIR sets an invalid discriminant and in the other case it doesn't, I'd say the interpreter is perfectly consistent. MIR building is not, but we should not paper over that on later phases.

So for both enum Test { A(u8), B(Empty) } and enum Test { A(u8), B(Empty), C }, the MIR in question is the same:

    bb2: {
        StorageLive(_3);
        _3 = move ((_1 as Some).0: Empty);
        StorageLive(_4);
        StorageLive(_5);
        _5 = move _3;
        ((_4 as B).0: Empty) = move _5;
        discriminant(_4) = 1;
        StorageDead(_5);
        StorageDead(_4);
        StorageDead(_3);
        goto -> bb3;
    }

playground link

Wesley Wiser (Oct 11 2019 at 13:39, on Zulip):

What's different is that if you eval discriminant(_4) = 1; in miri with enum Test { A(u8), B(Empty) } you get an ICE but if you do it with enum Test { A(u8), B(Empty), C } it runs fine.

RalfJ (Oct 12 2019 at 13:51, on Zulip):

See https://github.com/rust-lang/rust/pull/64890#issuecomment-541326824 : the MIR is the same but the layout is fundamentally different

RalfJ (Oct 12 2019 at 13:53, on Zulip):

I really think the right thing to do is turn these two asserts into throw_ub!(InvalidEnumDiscriminant). which const_prop ignores. then behavior is also consistent for const_prop ;)

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

I don't have an issue with that :)

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

Which two asserts did you mean specifically?

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

There's only 1 that I can get to trigger

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

I assume https://github.com/rust-lang/rust/blob/026447b9b0816bfc92d8072145a7a330ec8e3298/src/librustc_mir/interpret/place.rs#L1039 is one

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

I guess you mean https://github.com/rust-lang/rust/blob/026447b9b0816bfc92d8072145a7a330ec8e3298/src/librustc_mir/interpret/place.rs#L1047 as well?

RalfJ (Oct 12 2019 at 13:56, on Zulip):

I guess you mean https://github.com/rust-lang/rust/blob/026447b9b0816bfc92d8072145a7a330ec8e3298/src/librustc_mir/interpret/place.rs#L1047 as well?

yes

RalfJ (Oct 12 2019 at 13:56, on Zulip):

the two are morally the same

RalfJ (Oct 12 2019 at 13:56, on Zulip):

and there must be a third somewhere...

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

around 1070

RalfJ (Oct 12 2019 at 13:56, on Zulip):

here: https://github.com/rust-lang/rust/blob/026447b9b0816bfc92d8072145a7a330ec8e3298/src/librustc_mir/interpret/place.rs#L1071

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

yeah

RalfJ (Oct 12 2019 at 13:57, on Zulip):

basically rust has 3 ways to represent enums in memory

RalfJ (Oct 12 2019 at 13:57, on Zulip):

so both read _discriminant and write_discriminant have these 3 branches

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

Gotcha

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

That seems reasonable to me and then I can back out the change in const_prop

Wesley Wiser (Oct 12 2019 at 14:00, on Zulip):

The only other thing I wanted to talk with you about was:

Like, this code we are optimizing is UB if it ever gets executed. const-prop does not generally test for UB, but for some reason it has to detect this UB here... or so?

Wesley Wiser (Oct 12 2019 at 14:00, on Zulip):

I agree with the rest of your feedback and I can improve comments/whatever as needed

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

For that in particular, const_prop runs eagerly whenever we optimize MIR so it has to handle code that can't run at all

RalfJ (Oct 12 2019 at 14:01, on Zulip):

For that in particular, const_prop runs eagerly whenever we optimize MIR so it has to handle code that can't run at all

that was my understanding

RalfJ (Oct 12 2019 at 14:01, on Zulip):

I was just not happy with you needing extra checks before calling write_discriminant

Wesley Wiser (Oct 12 2019 at 14:02, on Zulip):

Sure, that's reasonable

Wesley Wiser (Oct 12 2019 at 14:02, on Zulip):

I think we have a good solution for that

RalfJ (Oct 12 2019 at 14:02, on Zulip):

but really what happened is that the Miri engine was exploiting that some kinds of MIR are never generated by our MIR builder, so in some places it does assert!

RalfJ (Oct 12 2019 at 14:02, on Zulip):

where if users could supply their own MIR those would all be ICEs and we'd need to have proper error handling

RalfJ (Oct 12 2019 at 14:02, on Zulip):

const_prop finds some of these places

Wesley Wiser (Oct 12 2019 at 14:02, on Zulip):

yeah, exactly

Wesley Wiser (Oct 12 2019 at 14:04, on Zulip):

Ok, I'll make those changes and add comments to the other stuff you pointed out

Wesley Wiser (Oct 12 2019 at 14:04, on Zulip):

Probably won't be until tomorrow or so

Wesley Wiser (Oct 12 2019 at 14:04, on Zulip):

Do you want me to make sure you get a chance to review before the PR is r+'d?

RalfJ (Oct 12 2019 at 14:05, on Zulip):

no it's okay, we can always follow-up if I still find something to complain about ;)

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

haha ok :)

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

Thanks for the feedback!

RalfJ (Oct 12 2019 at 14:06, on Zulip):

sure, thanks for taking it into account :)

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

ttyl!

Last update: Nov 17 2019 at 07:30UTC