Add discr_index to multi-variant layouts

[tmandry]

We remove the assumption that the discriminant is always field 0, in
preparations for layouts like generators where this is not (always) going to be
the case.

Specifically, upvars are going to go before the discriminant. In theory, it's possible to remove that assumption instead and keep the discriminant at field index 0, but one assumption or the other had to go :)

There is one place I know of in the debuginfo code where we'll still need to remove assumptions that the discriminant is the only field. I was planning on doing this along with the upcoming generator change, which will also include tests that exercise the code changing in this PR.

r? @eddyb
cc @oli-obk
cc @cramertj

[oli-obk]

What's the motivation for preferring this PR over making the generator layouts have field 0 always be the discriminant? Our fields do not have to be ordered by memory order, so the generator layouting code could always reorder the fields so that the discriminant comes first.

[oli-obk]

also use

rust/src/librustc/mir/mod.rs

Line 2004 in 1d4f0e7

pub struct Field {

here

[eddyb]

We're in rustc_target. Also, this whole module uses usize (if not u32) for field indices.

[oli-obk]

yea, this doesn't have to happen in this PR, and we can move the Field definition up into rustc_target

[tmandry]

What's the motivation for preferring this PR over making the generator layouts have field 0 always be the discriminant? Our fields do not have to be ordered by memory order, so the generator layouting code could always reorder the fields so that the discriminant comes first.

That's another possibility, but would have required another refactoring in other parts of the code that assume the "discriminant" is a field other than 0. I wasn't sure which approach would be better, maybe @eddyb knows?

[oli-obk]

would have required another refactoring in other parts of the code that assume the "discriminant" is a field other than 0

I'm confused. Isn't this what your PR is doing and the alternative is that no code outside the generator layout computation code would have to change?

[tmandry]

Sorry for not being more clear. A lot of the code that handles generators and/or closures of any kind assumes that the upvars start at field index 0. So AIUI, I would have to adjust all the lowering code to MIR, as well as how they are handled in debuginfo.

It doesn't seem obvious to me that that would be an easier or less invasive change, but then I really don't know.

[eddyb]

@oli-obk So what we want to get is basically a builtin version of this:

enum Foo {
    a: A,
    b: B,
    Bar(X),
    Baz(Y),
}

Those shared fields, a and b, already have "semantic" indices 0 and 1, and we wouldn't want to have to do ±1 everywhere to handle them - we used to have this with enum variants (because each variant layout had a copy of the discriminant at the start) and it was really annoying to work with (since it's "an off by one error by default").

In the case of generators, the upvars already have indices, so we have the same problem, with fields we don't want to complicate ourselves with renumbering.

---

An alternative to including the discriminant field index would be to say "the last field is the discriminant" instead of "field 0 is the discriminant".
Do we think that is an improvement over this PR?

[oli-obk]

Thanks, that motivation makes total sense.

I think encoding the last field would be very confusing. Having an explicit discriminant field index is the best solution indeed, I was just missing the context.

[cramertj]

@eddyb Sorry if i'm missing something, but this example here:

enum Foo {
    a: A,
    b: B,
    Bar(X),
    Baz(Y),
}

wouldn't be forwards-compatible with having some fields which were valid for multiple variants but not for all variants, which we want to support at some point, right? Is this just an artifact of how you wrote the syntax, or is this actually a limitation present in the design at this point?

[tmandry]

@cramertj We haven't discussed how to do this yet. I'm thinking we can either shoehorn it into this scheme (multiple vairants that have the same field at the same offset), or introduce an entirely new concept in the layout.

Either way, this still seems like a good first step to me.

[tmandry]

So @cramertj and I were talking about this some more. One approach that might be simpler in the long run is, rather than putting always-valid fields on the outer layout, we put them in the same place on every variant. Something like this:

enum Foo {
  Unresumed(A, B),
  YieldedOnce(A, B, X),
  YieldedTwice(A, B, Y),
}

Then we can leave those fields in place as we cycle through variants. It seems like this would be helpful later, when/if we want to optimize fields that are there across more than one variant. e.g.:

enum Foo {
  Unresumed(A, B),
  YieldedOnce(A, B, X),
  YieldedTwice(A, B, Y, Z),
  YieldedThrice(A, B, Z),
}

We could move Z around between the latter two variants, or generate padding for it within YieldedThrice so we don't have to, or whatever.

So, this approach does seem more general. What do people think about this? If it makes sense and it doesn't add considerable complexity to do it now, maybe we should go that route instead.

EDIT: Added second example for clarity

[eddyb]

@cramertj @tmandry In that example, the common fields are upvars not always-valid locals.
What @tmandry just said is what I expected we were talking about all along, wrt locals across yields.

[tmandry]

Okay, I was conflating those two things for some reason.

Would we ever want to do liveness analysis of non-Drop upvars and recover space from those as well?

In any case I think this PR is well-motivated and ready to merge.

[eddyb]

@bors r+

[bors]

📌 Commit ec2a144 has been approved by eddyb

[tmandry]

Rebased to make a dependent PR easier.

@bors r=eddyb

[bors]

📌 Commit 7c626a6 has been approved by eddyb

[bors]

⌛ Testing commit 7c626a6 with merge 9a612b2...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: eddyb
Pushing 9a612b2 to master...