Refactor enum layout calculation by susitsm · Pull Request #163749 · rust-lang/rust

susitsm · 2026-10-04T10:48:25Z

Moves layout calculation of enums to its own file, chunks large functions into smaller ones and makes some variables immutable

Motivation: Surely I wasn't the only one who tried to understand niche optimization and enum layout calculation, only to find a 500 lines long function full of mutable variables.

It is a +600-500 PR but reviewing commit-by-commit should be manageable. Commits compile and refactor a single part of layout calculation.

rustbot · 2026-10-04T10:48:31Z

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: codegen, compiler
codegen, compiler expanded to 77 candidates
Random selection from 18 candidates

jieyouxu

Thanks. Broadly seems like a good readability improvement. Some nits. I left review comments commit-by-commit so might show up weirdly.

@rustbot author

View changes since this review

jieyouxu · 2026-11-05T01:58:59Z

+                //
+                // How can this be nonzero when everything else is a ZST? `others_zst` is true here


Isn't others_zst about the other variants rather than fields of the largest variant?

E.g. if you have

#[repr(C)] struct Foo(u32, bool);

then Some variant of Option<Foo> would use the bool as the niche at offset 4. None would be the "others ZST" here.

jieyouxu · 2026-11-05T02:13:37Z

    align_abi: Align,
 }

+fn try_fixup_non_niche_variants<VariantIdx: Idx, FieldIdx: Idx>(


I'm a bit confused by this name 🤔 What this loop is doing is that we're trying to fit niche variants¹.

I think there might be a bit of a terminology thing here: say if we have

Option<Foo> Some { a: u32, b: bool } None

here None is the niche variant. Some with the bool would be the untagged_variant. Perhaps a better name for this would be try_fit_niche_variants? Or likely more accurate but maybe less straightforward, try_fit_other_variants 🤷 This could really use a doc comment TL;DR-ing the intent, i.e. trying to fit the variants that are not the untagged_variant.

Footnotes

That's a bit of a lie, this also handles absent² variants I think, so it's technically "everything that's not untagged_variant". But like calling it non_untagged_variants is probably trolling. Perhaps confusingly, this is specifically for niche encoding, so absent variants outside of niche_variants doesn't need a tag value. ↩

There's more nuance here that absent variants != ZST, and for a variant to qualify as an absent variant it seems that it needs to both satisfy (1) have at least one uninhabited field so it's never constructible, and (2) every field need to be 1-ZST. ↩

jieyouxu · 2026-11-05T02:34:45Z

+    let best_layout = match (tagged_layout, niche_filling_layout) {
+        (tl, Some(nl)) => {
+            // Pick the smaller layout; otherwise,
+            // pick the layout with the larger niche; otherwise,
+            // pick tagged as it has simpler codegen.
+            use cmp::Ordering::*;
+            let niche_size = |l: &LayoutData<FieldIdx, VariantIdx>| {
+                l.largest_niche.map_or(0, |n| n.available(dl))
+            };
+            match (tl.size.cmp(&nl.size), niche_size(&tl).cmp(&niche_size(&nl))) {
+                (Greater, _) => nl,
+                (Equal, Less) => nl,
+                _ => tl,
+            }
+        }
+        (tl, None) => tl,
+    };


This can probably pull out the niche_filling_layout as an early return, i.e. let Some(nl) = niche_filling_layout else { return Ok(tagged_layout) };. This match seems a bit unnecessarily complex.

jieyouxu · 2026-11-05T02:36:47Z

    Ok(best_layout)
 }

+fn constructable_variant<'a, VariantIdx, FieldIdx, F>(variant: &IndexSlice<FieldIdx, F>) -> bool


I think it's usually spelled constructible. Also, I would call this instead variant_is_inhabited, since I find it a bit more ambiguous as there's also the concept of "absent" in layout.

jieyouxu · 2026-11-05T02:42:52Z

-            }
-        }
-    }
+    let abi = calculate_tagged_abi(calculator, &layout_variants, &variants, size, align, tag);


Nit: you can probably lose the borrow on variants, since it's already &IndexSlice<...>

jieyouxu · 2026-11-05T02:50:01Z

+        // However, it is fine to make discr type however large (as an optimisation)
+        // after this point – we’ll just truncate the value we load in codegen.


Hm, I'd move this comment back to the call-site after sanity_check_ity, it's more like a positional invariant that holds after the sanity check on the call-site.

jieyouxu · 2026-11-05T02:52:20Z

    // If the alignment is not larger than the chosen discriminant size,
    // don't use the alignment as the final size.


This comment no longer belongs here, right? Maybe move it to the .max()?

jieyouxu · 2026-11-05T02:53:50Z

+    let prefix_align = if repr.c() {
+        variants
+            .iter()
+            .flatten()
+            .fold(min_ity.align(dl).abi, |prefix_align, field| prefix_align.max(field.align.abi))
+    } else {
+        min_ity.align(dl).abi
+    };


Maybe pull out a local, like let tag_align = min_ity.align(dl).abi;, it's a bit of a mouthful repeating here.

jieyouxu · 2026-11-05T03:03:09Z

    }
 }

+fn calculate_tagged_tag<FieldIdx, VariantIdx>(


This also adjusts variant field offsets/sizes 🤔 Maybe call this sth like finalize_tag_and_patch_variants (not a fantastic name, but sth to that effect).

jieyouxu · 2026-11-05T03:04:24Z

+    };
+    trace!(?min_negative, ?max_positive);
+
+    // Integer::discr_range_of_repr(tcx, ty, &repr, min, max);


This is probably leftover debugging code or sth, drop it?

rustbot · 2026-11-05T03:06:22Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

rustbot · 2026-11-05T13:39:39Z

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.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 4, 2026

rustbot assigned jieyouxu Oct 4, 2026

This comment has been minimized.

Sign in to view

susitsm force-pushed the refactor-enum-layout branch from c8a5f50 to 80bc421 Compare October 9, 2026 05:17

This comment has been minimized.

Sign in to view

WaffleLapkin self-assigned this Oct 16, 2026

This comment has been minimized.

Sign in to view

jieyouxu reviewed Nov 5, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2026

susitsm added 17 commits November 5, 2026 14:22

move layout_of_enum to its own file

50ca216

make calculate_niche_filling_layout a function instead of a closure

275aab3

calculate_niche_filling_layout: calculate_niche_abi

2bea906

calculate_niche_filling_layout: try_fixup_non_niche_variants

375db97

calculate_niche_filling_layout: remove redundant variables

eeafe7f

move tagged layout calculation to calculate_tagged_layout

0576a9f

calculate_tagged_layout: constructable_variant

3363ab5

calculate_tagged_layout: calculate_tagged_abi

4262d99

calculate_tagged_layout: remove discr_type variable

ccaf2a1

calculate_tagged_layout: make size immutable

d8d3981

calculate_niche_layout: remove all_indices

2fcd995

calculate_tagged_layout: sanity_check_ity

c08854b

calculate_tagged_layout: remove uninhabited

e7292b4

calculate_tagged_layout: make ity immutable

3bb351f

calculate_tagged_layout: make prefix_align immutable

b95feee

calculate_tagged_layout: calculate_tagged_tag

f5870e0

calculate_tagged_layout: calculate_tagged_min_ity

ce001ad

susitsm force-pushed the refactor-enum-layout branch from 80bc421 to ce001ad Compare November 5, 2026 13:39

		//
		// How can this be nonzero when everything else is a ZST? `others_zst` is true here

		// However, it is fine to make discr type however large (as an optimisation)
		// after this point – we’ll just truncate the value we load in codegen.

		// If the alignment is not larger than the chosen discriminant size,
		// don't use the alignment as the final size.

Conversation

susitsm commented Oct 4, 2026

Uh oh!

rustbot commented Oct 4, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jieyouxu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Nov 5, 2026

Uh oh!

rustbot commented Nov 5, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jieyouxu left a comment •

edited

Loading