Use alignment-sized integer for discriminant types

[Aatch]

The previous behaviour of using the smallest type possible caused LLVM
to treat padding too conservatively, causing poor codegen. This commit
changes the behaviour to use an alignment-sized integer as the
discriminant. This keeps types the same size, but helps LLVM understand
the data structure a little better, resulting in better codegen.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

[eddyb]

Do you have before/after IR examples? Preferably for some widely used enums.

[huonw]

Could you double check that the types are the same size, e.g.

use std::mem;
enum Foo {
    Bar(u8, u32),
    Baz
}

fn main() {
    println!("{} {}", mem::size_of::<Foo>(), mem::align_of::<Foo>())
}

prints 8 8 (on playpen), but it seems this change may force the size up to 16?

[Aatch]

@eddyb example of what? The representation or the codegen? For the codegen, that's kind of tricky, as it's hard to get a small (IR-wise anyway) example that exhibits the behaviour.

The motivation for this change was #19864

[Aatch]

@huonw it shouldn't change the size of the type as it basically just fills up the extra space that padding would have anyway. The logic is based on the code for generic_type_of. I'm just recompiling to double-check that nothing broke during the rebase (something did), but I'll double check to be sure.

In this case, the "payload" of Foo is 4-byte aligned because of the u32, so the discriminant should be an i32 in LLVM. Rather than an i8 followed by a [3 x i8].

[Aatch]

@huonw it did change the size, your example makes it 12 bytes though, not 16. I'll investigate.

[Aatch]

Ok, fixed the alignment for cases like the one @huonw showed. It now makes sure that the first relevant field has the same alignment as the overall type. A relevant field is one that either has a non-zero size or an alignment greater than 1. This is so it treats types like [u64,..0] correctly wrt alignment.

[alexcrichton]

Out of curiosity, could this sort of calculation be expressed as discr_size = max(minimum_discr, first_field_align)?

Could you also add some comments explaining why we're actually inflating the size of the discriminant in some cases?

Awesome work, thanks @Aatch!

[alexcrichton]

I think this could be simplified a bit with:

for st in fields.iter() {
    let field = st.fields.iter().skip(1).filter(...).next();
    if let Some(field) = field {
        // ...
    }
}

Basically just avoiding the indentation with the outer if statement and using skipe instead of the first .next()

[alexcrichton]

Just one minor nit, but other than that r=me, nice work @Aatch! I've always wanted this bug fixed!