Stream: t-compiler/major changes

Topic: Remove PredicateKind in favor of only Bin… compiler-team#397


triagebot (Jan 04 2021 at 17:58, on Zulip):

A new proposal has been announced: Remove PredicateKind in favor of only Binder<PredicateAtom> #397. It will be announced at the next meeting to try and draw attention to it, but usually MCPs are not discussed during triage meetings. If you think this would benefit from discussion amongst the team, consider proposing a design meeting.

Jack Huey (Jan 04 2021 at 17:59, on Zulip):

@lcnr @nikomatsakis

lcnr (Jan 04 2021 at 18:02, on Zulip):

:thumbs_up: i think this change is good but will leave it to @nikomatsakis to second as I want him to also be informed here

Jack Huey (Jan 04 2021 at 18:05, on Zulip):

I can understand that. I don't know when he'll have time to be around/review.

nikomatsakis (Jan 06 2021 at 14:13, on Zulip):

Heh, your concern is warranted :)

triagebot (Jan 06 2021 at 14:17, on Zulip):

@T-compiler: Proposal #397 has been seconded, and will be approved in 10 days if no objections are raised.

nikomatsakis (Jan 06 2021 at 14:17, on Zulip):

I trust the two of you on this, and the change makes sense I think

nikomatsakis (Jan 06 2021 at 14:17, on Zulip):

I skimmed the PR at least

Jack Huey (Jan 06 2021 at 21:15, on Zulip):

I updated based on your review

Jack Huey (Jan 06 2021 at 21:15, on Zulip):

@lcnr can you review?

Jack Huey (Jan 06 2021 at 21:16, on Zulip):

Is this uncontroversial/small enough that we don't need to wait for the full 10 day FCP?

lcnr (Jan 07 2021 at 10:52, on Zulip):

hm, I would have just waited until the fcp is over

lcnr (Jan 07 2021 at 10:53, on Zulip):

yeah, intend to review that pr

Jack Huey (Jan 07 2021 at 15:32, on Zulip):

So, I'm making the changes for your review @lcnr. But these end up being quite a heavy refactor in terms of code touched. So if these changes are uncontroversial and everybody is okay with it, I would prefer to try to land sooner rather than later to avoid bitrot.

lcnr (Jan 07 2021 at 15:50, on Zulip):

i have kinda complex feelings about this

lcnr (Jan 07 2021 at 15:51, on Zulip):

i also think that these changes are uncontroversial and don't expect any concerns here

lcnr (Jan 07 2021 at 15:53, on Zulip):

but there is value in having fairly strict rules for stuff like this which does get weakened by skipping the fcp for some mcps

varkor (Jan 07 2021 at 15:53, on Zulip):

Jack Huey said:

So, I'm making the changes for your review lcnr. But these end up being quite a heavy refactor in terms of code touched. So if these changes are uncontroversial and everybody is okay with it, I would prefer to try to land sooner rather than later to avoid bitrot.

I sympathise with this feeling, but I think the FCP process exists for a good reason, and it's not good to sidestep it.

varkor (Jan 07 2021 at 15:54, on Zulip):

I think unfortunate changes have been made in the past because a process like this was skipped.

Jack Huey (Jan 14 2021 at 13:26, on Zulip):

@lcnr is #80679 good other than FCP?

lcnr (Jan 14 2021 at 13:27, on Zulip):

still want to look at https://github.com/rust-lang/rust/pull/80679#discussion_r553482946

lcnr (Jan 14 2021 at 13:28, on Zulip):

will get to that this saturday probably

lcnr (Jan 14 2021 at 13:28, on Zulip):

apart from that yes

Jack Huey (Jan 16 2021 at 20:17, on Zulip):

When you get to this and decide if I need to remove that impl or not, I'll rebase.

Jack Huey (Jan 16 2021 at 22:42, on Zulip):

@lcnr in case you missed ^

lcnr (Jan 16 2021 at 22:42, on Zulip):

i did :laughing:

lcnr (Jan 16 2021 at 22:42, on Zulip):

will look now

lcnr (Jan 16 2021 at 23:01, on Zulip):

ok wow, how does this not blow up completely rn

lcnr (Jan 16 2021 at 23:02, on Zulip):

do we test rust on big endian systems?

lcnr (Jan 16 2021 at 23:03, on Zulip):

because if we did the current impl is incorrect or even unsound, wtf

simulacrum (Jan 16 2021 at 23:03, on Zulip):

We don't

simulacrum (Jan 16 2021 at 23:03, on Zulip):

Or at least not really

simulacrum (Jan 16 2021 at 23:03, on Zulip):

I think debian does and sometimes files reports

simulacrum (Jan 16 2021 at 23:04, on Zulip):

There's an issue open for s390x right now I think that's suspected as a big endian problem IIRC

lcnr (Jan 16 2021 at 23:04, on Zulip):

afaict decoding of types and predicates should be completely broken when using these systems

lcnr (Jan 16 2021 at 23:05, on Zulip):

and we just index into the void

lcnr (Jan 16 2021 at 23:06, on Zulip):

because we try reading the i32 discriminant as a usize, see that it's greater than 128 and just decide that it isn't worth it anymore

lcnr (Jan 16 2021 at 23:09, on Zulip):

or wait, we are saved because the default discriminant type is isize

lcnr (Jan 16 2021 at 23:09, on Zulip):

which is good

lcnr (Jan 16 2021 at 23:13, on Zulip):

@Jack Huey EncodableWithShorthand for Binder<PredicateKind> is only correct because Binder doesn't actually hold any information

Jack Huey (Jan 16 2021 at 23:14, on Zulip):

Okay, I'll just remove that impl then

lcnr (Jan 16 2021 at 23:14, on Zulip):

so we just skip it during encoding and the discriminant of PredicateKind is still the first thing we actually write

Jack Huey (Jan 16 2021 at 23:15, on Zulip):

Do we expect any perf change from that?

lcnr (Jan 16 2021 at 23:15, on Zulip):

yeah, probably

Jack Huey (Jan 16 2021 at 23:15, on Zulip):

Okay I'll also queue a perf run just to check

Jack Huey (Jan 16 2021 at 23:15, on Zulip):

Other than that, r=you?

lcnr (Jan 16 2021 at 23:16, on Zulip):

yeah, if perf isn't too bad

lcnr (Jan 16 2021 at 23:17, on Zulip):

we probably do want to reenable this for any relevant enum

Jack Huey (Jan 16 2021 at 23:17, on Zulip):

So PredicateKind?

lcnr (Jan 16 2021 at 23:17, on Zulip):

yeah, but that currently requires us to manually implement Decode

lcnr (Jan 16 2021 at 23:17, on Zulip):

which is probably annoying

Jack Huey (Jan 16 2021 at 23:18, on Zulip):

I'm a little confused on exactly what EncodableWithShortand is supposed to do

lcnr (Jan 16 2021 at 23:18, on Zulip):

enums are decoded as discr: isize | data ...

lcnr (Jan 16 2021 at 23:19, on Zulip):

the biggest possible discriminant is really small

lcnr (Jan 16 2021 at 23:20, on Zulip):

so we use the remaining usize::MAX - max_discr values to instead point back to a previous instance of the same value if it already exists

Jack Huey (Jan 16 2021 at 23:20, on Zulip):

ah I see

lcnr (Jan 16 2021 at 23:20, on Zulip):

so we would have whatever ...| discr_A | data | ... | position_of discr_A | ...

Jack Huey (Jan 16 2021 at 23:22, on Zulip):

I mean, there's also some overlap with the fact that predicates are interned, right?

lcnr (Jan 16 2021 at 23:23, on Zulip):

currently not

lcnr (Jan 16 2021 at 23:23, on Zulip):

rn we just decode the same predicate everytime

lcnr (Jan 16 2021 at 23:24, on Zulip):

not sure if that matters though as decoding stuff is quite fast i think

Jack Huey (Jan 16 2021 at 23:25, on Zulip):

but, I guess we don't even have to go through these hoops though? Like, encoding Predicate will try to encode &'tcx PredicateInner

Jack Huey (Jan 16 2021 at 23:26, on Zulip):

And that is essentially a &'tcx Binder<PredicateKind>

Jack Huey (Jan 16 2021 at 23:26, on Zulip):

So two predicate kinds that are the same shouldn't be decoded as two separate Predicates

Jack Huey (Jan 16 2021 at 23:27, on Zulip):

Unless I'm missing something

lcnr (Jan 16 2021 at 23:28, on Zulip):

(you probably still want to manually implement Decode for Predicate to directly encode Binder<PredicateKind> instead of PredicateInner)

Jack Huey (Jan 16 2021 at 23:28, on Zulip):

s/Decode/Encode?

lcnr (Jan 16 2021 at 23:29, on Zulip):

both :laughing:

Jack Huey (Jan 16 2021 at 23:29, on Zulip):

I'm pretty sure that is already implemented

lcnr (Jan 16 2021 at 23:30, on Zulip):

opened #81100 to prevent ourselves from accidentally introducing the bug i was worried about

lcnr (Jan 16 2021 at 23:30, on Zulip):

yeah, but it currently uses encode_with_shorthand

Jack Huey (Jan 16 2021 at 23:30, on Zulip):

ah gotcha

lcnr (Jan 16 2021 at 23:30, on Zulip):

where we instead now always fully Encode and Decode the Binder<PredicateKind>

Jack Huey (Jan 16 2021 at 23:32, on Zulip):

How much longer will you be around?

lcnr (Jan 16 2021 at 23:33, on Zulip):

not too much longer, probably less than an hour

Jack Huey (Jan 16 2021 at 23:33, on Zulip):

I can get the rebase/change done and you can double check. Shouldn't take more than 15 mins

lcnr (Jan 16 2021 at 23:35, on Zulip):

still have to wait for the perf run in case it's bad

Jack Huey (Jan 16 2021 at 23:36, on Zulip):

right, but assuming it's not, that's a simple r=lcnr

lcnr (Jan 16 2021 at 23:36, on Zulip):

i fear that this will have a noticeable impact on max-rss

Jack Huey (Jan 16 2021 at 23:36, on Zulip):

I'm not worried

Jack Huey (Jan 16 2021 at 23:38, on Zulip):

precisely because we also store PredicateKinds as interned

lcnr (Jan 16 2021 at 23:39, on Zulip):

but we don't encode them as such

Jack Huey (Jan 16 2021 at 23:39, on Zulip):

But it's simple to

lcnr (Jan 16 2021 at 23:40, on Zulip):

is it?

Jack Huey (Jan 16 2021 at 23:40, on Zulip):

Yeah, I think so

Jack Huey (Jan 16 2021 at 23:40, on Zulip):

I guess we'll see :)

Jack Huey (Jan 16 2021 at 23:48, on Zulip):

Wait, I might be a bit confused

Jack Huey (Jan 16 2021 at 23:49, on Zulip):

So how does DiscriminantKind work when enums have data

lcnr (Jan 16 2021 at 23:50, on Zulip):

the discriminant and the actual layout of an enum don't have to match

lcnr (Jan 16 2021 at 23:51, on Zulip):

DiscriminantKind is always isize even if the size of the enum is smaller

lcnr (Jan 16 2021 at 23:51, on Zulip):

unless you use an explicit repr attribute, in which case the size of the discriminant and the layout are the same

lcnr (Jan 16 2021 at 23:51, on Zulip):

so for Option<&Ty>

lcnr (Jan 16 2021 at 23:52, on Zulip):

discriminant_value(&None) is still 0 and discriminant_value(&Some(...)) is 1

lcnr (Jan 16 2021 at 23:52, on Zulip):

even though the actual representation of that type doesn't actually store the 1 anywhere

lcnr (Jan 16 2021 at 23:53, on Zulip):

and just checks for ptr != 0

Jack Huey (Jan 16 2021 at 23:55, on Zulip):

ah, okay

Jack Huey (Jan 16 2021 at 23:56, on Zulip):

So what exactly does is DiscriminantKind of a non-enum mean

lcnr (Jan 16 2021 at 23:56, on Zulip):

:shrug:

lcnr (Jan 16 2021 at 23:56, on Zulip):

it's pretty much meaningless

lcnr (Jan 16 2021 at 23:57, on Zulip):

so we just use 0u8

lcnr (Jan 16 2021 at 23:57, on Zulip):

because why not

lcnr (Jan 16 2021 at 23:57, on Zulip):

wanted to originally use () but eddy feared that we accidentally leak it into llvm which may interact badly with zero sized types

lcnr (Jan 16 2021 at 23:58, on Zulip):

for generators it's the current yield point

lcnr (Jan 16 2021 at 23:58, on Zulip):

or something like that

lcnr (Jan 16 2021 at 23:58, on Zulip):

and a u32 i think

Jack Huey (Jan 17 2021 at 00:01, on Zulip):

Ugh I'm trying to remember where I saw code for encoding/decoding interned data

lcnr (Jan 17 2021 at 00:02, on Zulip):

can't you repurpose the current impls

Jack Huey (Jan 17 2021 at 00:03, on Zulip):

not easily

lcnr (Jan 17 2021 at 00:04, on Zulip):

isn't encode e.kind().encode(encoder)

Jack Huey (Jan 17 2021 at 00:04, on Zulip):

well, yeah, we could just ignore interning

lcnr (Jan 17 2021 at 00:05, on Zulip):

and decode

fn decode(decoder: &mut D) -> Result<ty::Predicate<'tcx>, D::Error> {
        let predicate_kind =  ty::PredicateKind::decode(decoder)?;
        let predicate = decoder.tcx().mk_predicate(predicate_kind);
        Ok(predicate)
    }
lcnr (Jan 17 2021 at 00:05, on Zulip):

i still don't see how stuff being interned helps with encoding tbh :laughing:

Jack Huey (Jan 17 2021 at 00:07, on Zulip):

Hmm, maybe it's not helpful

Jack Huey (Jan 17 2021 at 00:08, on Zulip):

Let's keep it simple to start and if there are perf problems, I'll think

Jack Huey (Jan 17 2021 at 00:11, on Zulip):

also that decode isn't exactly right. Since we are decoding a Binder<PredicateKind>

Jack Huey (Jan 17 2021 at 00:13, on Zulip):

(and that's the bit that makes everything a bit more complicated)

lcnr (Jan 17 2021 at 00:14, on Zulip):

Decode::decode(self.kind(), decoder)?

lcnr (Jan 17 2021 at 00:14, on Zulip):

considering that Binder also implement decode

Jack Huey (Jan 17 2021 at 00:15, on Zulip):

I went with ty::Binder::<ty::PredicateKind<'tcx>>::decode(decoder) but yours is cleaner

Jack Huey (Jan 17 2021 at 00:17, on Zulip):

Decodable::decode(decoder)

lcnr (Jan 17 2021 at 00:17, on Zulip):

tru

Jack Huey (Jan 17 2021 at 00:18, on Zulip):

Ok, PR is updated

lcnr (Jan 17 2021 at 00:20, on Zulip):

think everything's looking good

lcnr (Jan 17 2021 at 00:20, on Zulip):

if perf and max-rss stays about the same r=me

Jack Huey (Jan 17 2021 at 08:15, on Zulip):

So looks like there indeed a perf/max-rss regression compared to previously.

Jack Huey (Jan 17 2021 at 08:16, on Zulip):

So...trying to think of a "fix"

Jack Huey (Jan 17 2021 at 08:22, on Zulip):

I think the right fix is to not automatically derive TyEncodable/TyDecodable for Binder

Jack Huey (Jan 17 2021 at 17:08, on Zulip):

Okay, made some changes and they're in between now. Somewhat mixed, but acceptable I think. @lcnr can you take a look when you get a chance?

lcnr (Jan 17 2021 at 17:27, on Zulip):

:thumbs_up:

Jack Huey (Jan 17 2021 at 17:32, on Zulip):

@lcnr done :)

triagebot (Jan 19 2021 at 18:31, on Zulip):

This proposal has been accepted: #397.

Last update: May 07 2021 at 07:15UTC