Stream: t-compiler

Topic: #63450 tracking discriminant type, Primitive vs Integer+s...


pnkfelix (Sep 16 2019 at 10:05, on Zulip):

To be honest, I'm not quite clear on the relative benefits of the three options you laid out in your 26 day old comment

pnkfelix (Sep 16 2019 at 10:07, on Zulip):

My assumption is that going to Integer + signedness would be fine. But you're more deeply acquainted with the code here, so I'd like to hear you out on why you want to go all the way to Integer.

pnkfelix (Sep 16 2019 at 10:07, on Zulip):

but in any case, maybe we can move this whole discussion to its own topic? I will do that now if I can

eddyb (Sep 16 2019 at 10:09, on Zulip):

oh yeah uhh sorry

eddyb (Sep 16 2019 at 10:10, on Zulip):

the reason to go all the way to Integer is that signedness should matter as least as possible, IMO, but also there is no (Integer, /*signed*/ bool) type so it'd be an annoying refactor (but maybe less annoying than having to deal with signedness on the fly?)

eddyb (Sep 16 2019 at 10:14, on Zulip):

@pnkfelix @RalfJ oh I'm silly there is something damning about losing signedness: these are both encoded as bytes and they both have isize (signed) high-level discriminants: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c140a4df209bececc5471d376fc2e3d0

pnkfelix (Sep 16 2019 at 10:14, on Zulip):

I guess; your PR itself says that going all the way to Integer only seemed like a slight simplification, which makes me wary of trying to push this through, in terms of effort expended vs expected value.

pnkfelix (Sep 16 2019 at 10:14, on Zulip):

pnkfelix RalfJ oh I'm silly there is something damning about losing signedness: these are both encoded as bytes and they both have isize (signed) high-level discriminants: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c140a4df209bececc5471d376fc2e3d0

eek

pnkfelix (Sep 16 2019 at 10:15, on Zulip):

okay then seems like that ... answers your Question then?

eddyb (Sep 16 2019 at 10:15, on Zulip):

yeah I somehow didn't think to check this one thing that would've made it clear...

pnkfelix (Sep 16 2019 at 10:15, on Zulip):

that is, I assume the PR you posted mishandles this case? And therefore we should go with option 2 (Integer + signedness)

eddyb (Sep 16 2019 at 10:15, on Zulip):

if we want to change the code at all

eddyb (Sep 16 2019 at 10:16, on Zulip):

I'm not sure how much the status quo inconvenienced @RalfJ

eddyb (Sep 16 2019 at 10:21, on Zulip):

@RalfJ it seems like you should be able to just remove this line and land the PR sigh <https://github.com/rust-lang/rust/pull/63448/files#diff-47c36b7a2c9437eb6f382b25d46a3c54R688>

eddyb (Sep 16 2019 at 10:21, on Zulip):

oh there's another copy here <https://github.com/rust-lang/rust/pull/63448/files#diff-9c5f25e0d0302fa34791d33795e30781R1061>

RalfJ (Sep 16 2019 at 10:30, on Zulip):

Well I wouldn't remove it but replace it by a comment and probably want to factor that match into a helper method? what would be a good place for that method?

eddyb (Sep 16 2019 at 10:46, on Zulip):

@RalfJ presumably in here https://github.com/rust-lang/rust/blob/master/src/librustc/ty/layout.rs#L129

eddyb (Sep 16 2019 at 10:46, on Zulip):

@RalfJ wait, does the type really matter? can't you just use the size?

eddyb (Sep 16 2019 at 10:47, on Zulip):

I guess it matter for extending it but not for the addition

eddyb (Sep 16 2019 at 10:48, on Zulip):

@RalfJ oh and I guess this is wrong https://github.com/rust-lang/rust/pull/63448/files#diff-47c36b7a2c9437eb6f382b25d46a3c54R706

eddyb (Sep 16 2019 at 10:49, on Zulip):

variants_start might not fit in the discriminant

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

RalfJ wait, does the type really matter? can't you just use the size?

size and signedess

RalfJ (Sep 16 2019 at 10:57, on Zulip):

variants_start might not fit in the discriminant

@eddyb then this would be the wrong thing, right?
https://github.com/rust-lang/rust/pull/63448/files#diff-47c36b7a2c9437eb6f382b25d46a3c54R700
this (at least in debug builds, not sure about release builds) will panic if it doesn't fit

RalfJ (Sep 16 2019 at 10:57, on Zulip):

what should it do instead?

eddyb (Sep 16 2019 at 10:58, on Zulip):

the only operation you need to do is the subtraction

eddyb (Sep 16 2019 at 10:59, on Zulip):

the result of that should be an unsigned integer, so you can just zero-extend without knowing the signedness (maybe, I'm not 100% sure)

RalfJ (Sep 16 2019 at 10:59, on Zulip):

the only operation you need to do is the subtraction

I am doing two operations though?

eddyb (Sep 16 2019 at 11:00, on Zulip):

sorry, the only operation you need to do in a special way is the subtraction

RalfJ (Sep 16 2019 at 11:00, on Zulip):

"special"? I am doing both using machine arithmetic right now, that seems most obvious?

eddyb (Sep 16 2019 at 11:00, on Zulip):

once you've done the subtraction wrapping at the right size, you get a positive (or 0) distance from variants_start to the index of a variant

RalfJ (Sep 16 2019 at 11:00, on Zulip):

and even for just doing those I need to pick a signedness and it seems preferrable to pick the right one?

eddyb (Sep 16 2019 at 11:01, on Zulip):

well addition and subtraction don't care about signedness :)

RalfJ (Sep 16 2019 at 11:01, on Zulip):

fair

RalfJ (Sep 16 2019 at 11:01, on Zulip):

still, I dont see a reason not to extract the signedness^^

eddyb (Sep 16 2019 at 11:02, on Zulip):

anyway adjusted_discr is badly named. it's a variant index at that point

RalfJ (Sep 16 2019 at 11:02, on Zulip):

so you are saying I should do variant_index_val - variants_start_val using host, not machine, arithmetic?

eddyb (Sep 16 2019 at 11:02, on Zulip):

are we looking at a different operation?

eddyb (Sep 16 2019 at 11:03, on Zulip):

anyway yeah the operation with variants_start should just be on u128 and it can never overflow so you don't need to use wrapping_ either

eddyb (Sep 16 2019 at 11:03, on Zulip):

if variants_start <= adjusted_discr && adjusted_discr <= variants_end { seems wrong btw

eddyb (Sep 16 2019 at 11:03, on Zulip):

I forget if we've discussed this

RalfJ (Sep 16 2019 at 11:03, on Zulip):

anyway adjusted_discr is badly named. it's a variant index at that point

so maybe variant_index_relative or so?

RalfJ (Sep 16 2019 at 11:03, on Zulip):

are we looking at a different operation?

I looked ath the link you posted

RalfJ (Sep 16 2019 at 11:04, on Zulip):

which was in the write code

eddyb (Sep 16 2019 at 11:04, on Zulip):

oh whoops

eddyb (Sep 16 2019 at 11:04, on Zulip):

I am now staring at the read code

eddyb (Sep 16 2019 at 11:04, on Zulip):

(not sure how I got here tbh)

eddyb (Sep 16 2019 at 11:04, on Zulip):

anyway it's not even relative once you add variants_start

eddyb (Sep 16 2019 at 11:05, on Zulip):

hmmm okay maybe you do need wrapping_ ops for the host stuff since your range check handles the dataful thing

eddyb (Sep 16 2019 at 11:05, on Zulip):

i.e. it may overflow when it's the dataful case

eddyb (Sep 16 2019 at 11:05, on Zulip):

I wish I could remember any of this

RalfJ (Sep 16 2019 at 11:07, on Zulip):

anyway it's not even relative once you add variants_start

yeah, I meant only the first intermediate value

RalfJ (Sep 16 2019 at 11:07, on Zulip):

if variants_start <= adjusted_discr && adjusted_discr <= variants_end { seems wrong btw

it does?

RalfJ (Sep 16 2019 at 11:07, on Zulip):

hmmm okay maybe you do need wrapping_ ops for the host stuff since your range check handles the dataful thing

let me ask the other way around -- we do machine arithmetic at run-time for this, right? so, using machine arithmetic is correct?

RalfJ (Sep 16 2019 at 11:08, on Zulip):

I currently have no interest in optimizing this code ;)

RalfJ (Sep 16 2019 at 11:09, on Zulip):

anyway adjusted_discr is badly named. it's a variant index at that point

I renamed: https://github.com/rust-lang/rust/pull/63448/commits/b73f1a51dcd1011129dc5c5c55a7577134410dbc

eddyb (Sep 16 2019 at 11:10, on Zulip):

it's not about optimization

eddyb (Sep 16 2019 at 11:10, on Zulip):

the runtime behavior was wrong until recently

eddyb (Sep 16 2019 at 11:10, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_ssa/mir/place.rs#L294-L301

eddyb (Sep 16 2019 at 11:11, on Zulip):

only one of the operations can be performed at the niche size, the other has to be performed at something capable of fitting all variant indices

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

and whether it's a niche or dataful variant is checked not on the final variant index but on the relative one

eddyb (Sep 16 2019 at 11:13, on Zulip):

@RalfJ maybe you can use https://github.com/rust-lang/rust/pull/62584/files#diff-dac0b3d5a19b291538d6f171b7e8eb8b?

eddyb (Sep 16 2019 at 11:13, on Zulip):

as a miri test

eddyb (Sep 16 2019 at 11:13, on Zulip):

I expect miri to get this very wrong atm

RalfJ (Sep 16 2019 at 11:19, on Zulip):

we have some tests in https://github.com/rust-lang/miri/pull/903 that you supplied earlier

RalfJ (Sep 16 2019 at 11:20, on Zulip):

I can certainly add that test as well. you are saying that will still fail even with the PR?

RalfJ (Sep 16 2019 at 11:20, on Zulip):

only one of the operations can be performed at the niche size, the other has to be performed at something capable of fitting all variant indices

and which one would that be?

RalfJ (Sep 16 2019 at 12:48, on Zulip):

@eddyb that test case you suggested causes an ICE. good catch, I guess. ;)

RalfJ (Sep 16 2019 at 12:49, on Zulip):

so for writing the discriminant, I assume computing the relative variant index should happen at max precision? and only adding niche_start_val should happen with overflow?

RalfJ (Sep 16 2019 at 13:06, on Zulip):

@eddyb I updated the PR, I think that's better now? I am still confused by your "variants_start <= adjusted_discr && adjusted_discr <= variants_end seems wrong" though.

eddyb (Sep 17 2019 at 13:31, on Zulip):

@RalfJ does the test pass now? because at this point my pedantry has outstayed its welcome :P

RalfJ (Sep 17 2019 at 20:56, on Zulip):

yes, the tests added in https://github.com/rust-lang/miri/pull/903 all pass

RalfJ (Sep 17 2019 at 20:57, on Zulip):

@eddyb I am still not sure though what you meant by
"variants_start <= adjusted_discr && adjusted_discr <= variants_end seems wrong"

eddyb (Sep 18 2019 at 11:32, on Zulip):

it can be fine, I just got confused and had assumed something about the operations before

RalfJ (Sep 18 2019 at 20:46, on Zulip):

kk

Last update: Nov 22 2019 at 04:35UTC