Enum Layout Optimization: Variant fields reordering around niche.

[Pinguimmar]

Discussion

Following the discussion in this issue, we looked into the matter and tried to implement it.

First, as @zachs18 said: for this example to work we would need to change the enum tag attribution strategy to create the lowest tag possible (instead of the biggest that does not increase the final size). Because currently this Inner tag is being created with 4bytes.

pub enum Inner {
    A(u32),
    B(u32),
}

pub enum Outer {
    A(Inner),
    B(u32, u8),
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=519748d9a2c1b53ee38c46bff89bf375

The thing is, that looks to be done for optimizations in the LLVM :

rust/compiler/rustc_abi/src/layout.rs

Lines 894 to 901 in 99b9a88

	// Check to see if we should use a different type for the
	// discriminant. We can safely use a type with the same size
	// as the alignment of the first field of each variant.
	// We increase the size of the discriminant to avoid LLVM copying
	// padding when it doesn't need to. This normally causes unaligned
	// load/stores and excessive memcpy/memset operations. By using a
	// bigger integer size, LLVM can be sure about its contents and
	// won't be so conservative.

Removing this optimization looks more like a regression, so instead let's use another example were the tag does not use all all space in alignment.

enum Inner {
    A(u32, u8),
    B(u32),
}

enum Outer {
    A(Inner),
    B(u32, u8),    
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=e8f7bc1e568a63cc698d2b26036d817f
In this example because of u8 in the Inner, the tag will only be 1 byte.
[tag(1byte)] [u8] [padding(2bytes)] [u32]
(8bytes in total)

Now in this example we could try to reorder fields. Outer::B could fit like this:
[tag(1b)] [u8(1b)] [pad(2b)] [u32(4b)], which as we can see fits in 8bytes.

Implementation

Our solution is basically to place the variant fields all around the niche of the "biggest variant" instead of just trying to place the whole struct on the left or right, when the normal method does not work.

We are not sure of this, but since this changes the layout of a basic structure of Rust, we implemented this in a way that it only runs if compiled with the flag "-Zunstable-options --edition future".
Considering this, and that we needed to change the logic of selecting the biggest variant, and as to not complicate calculate_niche_filling_layout we added a closure calculate_niche_filling_layout_repacked that is based on calculate_niche_filling_layout with some changes in the biggest variant choice and the calling of the new function that tries repack fields around niche.
https://github.com/Pinguimmar/rust/blob/c34d41888b2f9167bac60167afe1e616e130a0d3/compiler/rustc_abi/src/layout.rs#L742-L752
Thinking about it now, it is repeating quite some code so we should probably change it, but for now we will create this PR with the current version.

Choice of Variant to serve as niche-tag

Before, choosing one of the max-size variants was good enough (because we use the whole struct as a unit), but now since we will be reordering the fields, it is not sufficient. The size of a variant includes alignments, but since we will be placing the fields individually, that padding space may disappear, which would mean that the variant size is not correct. With this in mind we substituted it by a for loop that runs through the the variants that have the biggest size and contain niche.
https://github.com/Pinguimmar/rust/blob/c34d41888b2f9167bac60167afe1e616e130a0d3/compiler/rustc_abi/src/layout.rs#L778-L785

We accept the first max-size variant that works, as trying all max-size variants as niche-tag candidates would not optimize the final enum size and we deem not worth it, but this is debatable.

Repacking fields Function

We created a function that tries to fit the fields around the niche. Instead of handling the variant as a whole struct we treat each field individually. We consider the niche as a fixed offset and occupied, and then for each field we try to put it in the first possible position. The fields are placed by decreasing order, so as to avoid paddings.

Note that this function is only called if any of the variants cannot fit as a whole before and after the niche, (in a give up case). We implemented it in this way so as to not increase complexity for the cases that currently work.
https://github.com/Pinguimmar/rust/blob/c34d41888b2f9167bac60167afe1e616e130a0d3/compiler/rustc_abi/src/layout.rs#L824-L852

Closes #147341

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dingxiangfei2009 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: codegen, compiler
• codegen, compiler expanded to 73 candidates
• Random selection from 17 candidates

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Pinguimmar]

This seems not to be a code problem, so I will probably do an empty commit to force re-run of tests as I do not have permission to re-run them manually. But first I will check if there's a more appropriate way.

[Pinguimmar]

@rustbot assign @dingxiangfei2009

[Pinguimmar]

@dingxiangfei2009 just as a reminder.

[Exonorid]

Rust makes no guarantees about enum layout stability, so I don't see a reason to put this behind a future version.

View changes since the review

[Pinguimmar]

Ohh thanks, it makes sense. I had seen that in some discussion but still we were not sure about it. Thanks, we will patch it as soon as possible.😁

[rustbot]

Changes to the size of AST and/or HIR nodes.

cc @nnethercote