From 23d815c679e777c57632a914ea600b9b66a46071 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <ogoffart@woboq.com> Date: Thu, 26 Mar 2020 09:22:17 +0100 Subject: [PATCH 1/7] Put the largest niche first --- src/librustc_middle/ty/layout.rs | 64 ++++++++++++------- .../ui/consts/const-eval/const_transmute.rs | 2 + src/test/ui/print_type_sizes/padding.stdout | 6 +- 3 files changed, 47 insertions(+), 25 deletions(-) diff --git a/src/librustc_middle/ty/layout.rs b/src/librustc_middle/ty/layout.rs index 1bb338d43ad0a..5de7fe9df34fa 100644 --- a/src/librustc_middle/ty/layout.rs +++ b/src/librustc_middle/ty/layout.rs @@ -283,6 +283,23 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { let mut align = if pack.is_some() { dl.i8_align } else { dl.aggregate_align }; + let largest_niche_index = if matches!(kind, StructKind::Prefixed{..}) || repr.hide_niche() { + None + } else { + fields + .iter() + .enumerate() + .filter_map(|(i, &field)| field.largest_niche.as_ref().map(|n| (i, n))) + .max_by_key(|(_, niche)| (niche.available(dl), cmp::Reverse(niche.offset))) + .map(|(i, _)| i as u32) + }; + + // inverse_memory_index holds field indices by increasing memory offset. + // That is, if field 5 has offset 0, the first element of inverse_memory_index is 5. + // We now write field offsets to the corresponding offset slot; + // field 5 with offset 0 puts 0 in offsets[5]. + // At the bottom of this function, we invert `inverse_memory_index` to + // produce `memory_index` (see `invert_mapping`). let mut inverse_memory_index: Vec<u32> = (0..fields.len() as u32).collect(); let optimize = !repr.inhibit_struct_field_reordering_opt(); @@ -296,10 +313,15 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { match kind { StructKind::AlwaysSized | StructKind::MaybeUnsized => { optimizing.sort_by_key(|&x| { - // Place ZSTs first to avoid "interesting offsets", - // especially with only one or two non-ZST fields. let f = &fields[x as usize]; - (!f.is_zst(), cmp::Reverse(field_align(f))) + ( + // Place ZSTs first to avoid "interesting offsets", + // especially with only one or two non-ZST fields. + !f.is_zst(), + cmp::Reverse(field_align(f)), + // Try to put the largest niche earlier. + Some(x) != largest_niche_index, + ) }); } StructKind::Prefixed(..) => { @@ -308,20 +330,25 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { optimizing.sort_by_key(|&x| field_align(&fields[x as usize])); } } + // Rotate index array to put the largest niche first. + // Since it is already the first amongst the types with the same alignement, + // this will just move some of the potential padding within the structure. + if let (Some(niche_index), StructKind::AlwaysSized) = (largest_niche_index, kind) { + // ZSTs are always first, and the largest niche is not one, so we can unwrap + let first_non_zst = inverse_memory_index + .iter() + .position(|&x| !fields[x as usize].is_zst()) + .unwrap(); + let non_zsts = &mut inverse_memory_index[first_non_zst..]; + let pivot = non_zsts.iter().position(|&x| x == niche_index).unwrap(); + non_zsts.rotate_left(pivot); + } } - // inverse_memory_index holds field indices by increasing memory offset. - // That is, if field 5 has offset 0, the first element of inverse_memory_index is 5. - // We now write field offsets to the corresponding offset slot; - // field 5 with offset 0 puts 0 in offsets[5]. - // At the bottom of this function, we invert `inverse_memory_index` to - // produce `memory_index` (see `invert_mapping`). - let mut sized = true; let mut offsets = vec![Size::ZERO; fields.len()]; let mut offset = Size::ZERO; let mut largest_niche = None; - let mut largest_niche_available = 0; if let StructKind::Prefixed(prefix_size, prefix_align) = kind { let prefix_align = @@ -351,18 +378,11 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { debug!("univariant offset: {:?} field: {:#?}", offset, field); offsets[i as usize] = offset; - - if !repr.hide_niche() { - if let Some(mut niche) = field.largest_niche.clone() { - let available = niche.available(dl); - if available > largest_niche_available { - largest_niche_available = available; - niche.offset += offset; - largest_niche = Some(niche); - } - } + if largest_niche_index == Some(i) { + let mut niche = field.largest_niche.clone().unwrap(); + niche.offset += offset; + largest_niche = Some(niche) } - offset = offset.checked_add(field.size, dl).ok_or(LayoutError::SizeOverflow(ty))?; } diff --git a/src/test/ui/consts/const-eval/const_transmute.rs b/src/test/ui/consts/const-eval/const_transmute.rs index f0e1d8263022b..59d53c3095120 100644 --- a/src/test/ui/consts/const-eval/const_transmute.rs +++ b/src/test/ui/consts/const-eval/const_transmute.rs @@ -33,8 +33,10 @@ impl Drop for Foo { } #[derive(Copy, Clone)] +#[repr(C)] struct Fat<'a>(&'a Foo, &'static VTable); +#[repr(C)] struct VTable { drop: Option<for<'a> fn(&'a mut Foo)>, size: usize, diff --git a/src/test/ui/print_type_sizes/padding.stdout b/src/test/ui/print_type_sizes/padding.stdout index 9afdf76245df7..358033a7ccc9e 100644 --- a/src/test/ui/print_type_sizes/padding.stdout +++ b/src/test/ui/print_type_sizes/padding.stdout @@ -17,7 +17,7 @@ print-type-size field `.0`: 1 bytes print-type-size padding: 2 bytes print-type-size field `.1`: 4 bytes, alignment: 4 bytes print-type-size type: `S`: 8 bytes, alignment: 4 bytes -print-type-size field `.g`: 4 bytes -print-type-size field `.a`: 1 bytes print-type-size field `.b`: 1 bytes -print-type-size end padding: 2 bytes +print-type-size field `.a`: 1 bytes +print-type-size padding: 2 bytes +print-type-size field `.g`: 4 bytes, alignment: 4 bytes From 059b19a761c6ad7890a9b0ffbc85698966cc1528 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <ogoffart@woboq.com> Date: Wed, 15 Apr 2020 19:54:05 +0200 Subject: [PATCH 2/7] Fix the re-ordering of the field to put the niche first --- src/librustc_middle/ty/layout.rs | 7 +++++-- src/test/ui/type-sizes.rs | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/librustc_middle/ty/layout.rs b/src/librustc_middle/ty/layout.rs index 5de7fe9df34fa..6d16c2a8ddc74 100644 --- a/src/librustc_middle/ty/layout.rs +++ b/src/librustc_middle/ty/layout.rs @@ -330,8 +330,8 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { optimizing.sort_by_key(|&x| field_align(&fields[x as usize])); } } - // Rotate index array to put the largest niche first. - // Since it is already the first amongst the types with the same alignement, + // Rotate index array to put the largest niche first. Then reverse the ones with larger + // alignment. Since it is already the first amongst the types with the same alignment, // this will just move some of the potential padding within the structure. if let (Some(niche_index), StructKind::AlwaysSized) = (largest_niche_index, kind) { // ZSTs are always first, and the largest niche is not one, so we can unwrap @@ -342,6 +342,9 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { let non_zsts = &mut inverse_memory_index[first_non_zst..]; let pivot = non_zsts.iter().position(|&x| x == niche_index).unwrap(); non_zsts.rotate_left(pivot); + let pivot = non_zsts.len() - pivot; + non_zsts[pivot..].reverse(); + debug_assert_eq![non_zsts[0], niche_index]; } } diff --git a/src/test/ui/type-sizes.rs b/src/test/ui/type-sizes.rs index 6a3f3c98f127a..45a702653f506 100644 --- a/src/test/ui/type-sizes.rs +++ b/src/test/ui/type-sizes.rs @@ -145,4 +145,8 @@ pub fn main() { assert_eq!(size_of::<Option<Option<(&(), bool)>>>(), size_of::<(bool, &())>()); assert_eq!(size_of::<Option<Option2<bool, &()>>>(), size_of::<(bool, &())>()); assert_eq!(size_of::<Option<Option2<&(), bool>>>(), size_of::<(bool, &())>()); + + struct S1{ a: u16, b: std::num::NonZeroU16, c: u16, d: u8, e: u32, f: u64, g:[u8;2] } + assert_eq!(size_of::<S1>(), 24); + assert_eq!(size_of::<Option<S1>>(), 24); } From d58273b5f1bb007552aae9f48575c34bb32b9247 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <ogoffart@woboq.com> Date: Fri, 27 Mar 2020 11:17:26 +0100 Subject: [PATCH 3/7] Use the niche optimisation if other enum variant are small enough Reduce the size of enums which have one bigger variant with a niche --- src/librustc_middle/ty/layout.rs | 129 ++++++++++++++---- src/librustc_middle/ty/sty.rs | 4 +- src/librustc_mir_build/hair/mod.rs | 2 +- .../ui/print_type_sizes/niche-filling.stdout | 6 +- src/test/ui/print_type_sizes/padding.stdout | 26 ++-- src/test/ui/type-sizes.rs | 18 +++ 6 files changed, 137 insertions(+), 48 deletions(-) diff --git a/src/librustc_middle/ty/layout.rs b/src/librustc_middle/ty/layout.rs index 6d16c2a8ddc74..1c400d0bbfadd 100644 --- a/src/librustc_middle/ty/layout.rs +++ b/src/librustc_middle/ty/layout.rs @@ -898,48 +898,103 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { if !def.repr.inhibit_enum_layout_opt() && no_explicit_discriminants { let mut dataful_variant = None; let mut niche_variants = VariantIdx::MAX..=VariantIdx::new(0); + let mut max_size = Size::ZERO; + let mut second_max_size = Size::ZERO; + + // The size computations below assume that the padding is minimum. + // This is the case when fields are re-ordered. + let struct_reordering_opt = !def.repr.inhibit_struct_field_reordering_opt(); + if struct_reordering_opt { + let mut extend_niche_range = |d| { + niche_variants = + *niche_variants.start().min(&d)..=*niche_variants.end().max(&d); + }; + let mut align = dl.aggregate_align; - // Find one non-ZST variant. - 'variants: for (v, fields) in variants.iter_enumerated() { - if absent(fields) { - continue 'variants; + // Find the largest and second largest variant. + for (v, fields) in variants.iter_enumerated() { + if absent(fields) { + continue; + } + let mut size = Size::ZERO; + for &f in fields { + align = align.max(f.align); + size += f.size; + } + if size > max_size { + second_max_size = max_size; + max_size = size; + if let Some(d) = dataful_variant { + extend_niche_range(d); + } + dataful_variant = Some(v); + } else if size == max_size { + if let Some(d) = dataful_variant { + extend_niche_range(d); + } + dataful_variant = None; + extend_niche_range(v); + } else { + second_max_size = second_max_size.max(size); + extend_niche_range(v); + } } - for f in fields { - if !f.is_zst() { - if dataful_variant.is_none() { - dataful_variant = Some(v); - continue 'variants; - } else { - dataful_variant = None; - break 'variants; + if !max_size.is_aligned(align.abi) { + // Don't perform niche optimisation if there is padding anyway + dataful_variant = None; + } + } else { + // Find one non-ZST variant. + 'variants: for (v, fields) in variants.iter_enumerated() { + if absent(fields) { + continue 'variants; + } + for f in fields { + if !f.is_zst() { + if dataful_variant.is_none() { + dataful_variant = Some(v); + continue 'variants; + } else { + dataful_variant = None; + break 'variants; + } } + niche_variants = *niche_variants.start().min(&v)..=v } } - niche_variants = *niche_variants.start().min(&v)..=v; } if niche_variants.start() > niche_variants.end() { dataful_variant = None; } - if let Some(i) = dataful_variant { + if let Some(dataful_variant) = dataful_variant { let count = (niche_variants.end().as_u32() - niche_variants.start().as_u32() + 1) as u128; // Find the field with the largest niche - let niche_candidate = variants[i] + let niche_candidate = variants[dataful_variant] .iter() .enumerate() .filter_map(|(j, &field)| Some((j, field.largest_niche.as_ref()?))) - .max_by_key(|(_, niche)| niche.available(dl)); + .max_by_key(|(_, n)| (n.available(dl), cmp::Reverse(n.offset))) + .and_then(|(field_index, niche)| { + // make sure there is enough room for the other variants + if struct_reordering_opt + && max_size - (niche.offset + niche.scalar.value.size(dl)) + < second_max_size + { + return None; + } + Some((field_index, niche, niche.reserve(self, count)?)) + }); if let Some((field_index, niche, (niche_start, niche_scalar))) = - niche_candidate.and_then(|(field_index, niche)| { - Some((field_index, niche, niche.reserve(self, count)?)) - }) + niche_candidate { let mut align = dl.aggregate_align; + let prefix = niche.offset + niche.scalar.value.size(dl); let st = variants .iter_enumerated() .map(|(j, v)| { @@ -947,7 +1002,14 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { ty, v, &def.repr, - StructKind::AlwaysSized, + if j == dataful_variant || second_max_size == Size::ZERO { + StructKind::AlwaysSized + } else { + StructKind::Prefixed( + prefix, + Align::from_bytes(1).unwrap(), + ) + }, )?; st.variants = Variants::Single { index: j }; @@ -957,13 +1019,22 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { }) .collect::<Result<IndexVec<VariantIdx, _>, _>>()?; - let offset = st[i].fields.offset(field_index) + niche.offset; - let size = st[i].size; + let niche_offset = if struct_reordering_opt { + debug_assert_eq!( + st[dataful_variant].fields.offset(field_index), + Size::ZERO + ); + niche.offset + } else { + st[dataful_variant].fields.offset(field_index) + niche.offset + }; + let size = st[dataful_variant].size; + debug_assert!(st.iter().all(|v| { v.size <= size })); let abi = if st.iter().all(|v| v.abi.is_uninhabited()) { Abi::Uninhabited - } else { - match st[i].abi { + } else if second_max_size == Size::ZERO { + match st[dataful_variant].abi { Abi::Scalar(_) => Abi::Scalar(niche_scalar.clone()), Abi::ScalarPair(ref first, ref second) => { // We need to use scalar_unit to reset the @@ -971,7 +1042,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { // primitive, because only the niche is // guaranteed to be initialised, not the // other primitive. - if offset.bytes() == 0 { + if niche_offset.bytes() == 0 { Abi::ScalarPair( niche_scalar.clone(), scalar_unit(second.value), @@ -985,16 +1056,18 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { } _ => Abi::Aggregate { sized: true }, } + } else { + Abi::Aggregate { sized: true } }; let largest_niche = - Niche::from_scalar(dl, offset, niche_scalar.clone()); + Niche::from_scalar(dl, niche_offset, niche_scalar.clone()); return Ok(tcx.intern_layout(Layout { variants: Variants::Multiple { discr: niche_scalar, discr_kind: DiscriminantKind::Niche { - dataful_variant: i, + dataful_variant, niche_variants, niche_start, }, @@ -1002,7 +1075,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { variants: st, }, fields: FieldsShape::Arbitrary { - offsets: vec![offset], + offsets: vec![niche_offset], memory_index: vec![0], }, abi, diff --git a/src/librustc_middle/ty/sty.rs b/src/librustc_middle/ty/sty.rs index 248a2095d0a2c..6fe63212a7a33 100644 --- a/src/librustc_middle/ty/sty.rs +++ b/src/librustc_middle/ty/sty.rs @@ -2204,7 +2204,7 @@ pub struct Const<'tcx> { } #[cfg(target_arch = "x86_64")] -static_assert_size!(Const<'_>, 48); +static_assert_size!(Const<'_>, if cfg!(bootstrap) { 48 } else { 40 }); impl<'tcx> Const<'tcx> { /// Literals and const generic parameters are eagerly converted to a constant, everything else @@ -2432,7 +2432,7 @@ pub enum ConstKind<'tcx> { } #[cfg(target_arch = "x86_64")] -static_assert_size!(ConstKind<'_>, 40); +static_assert_size!(ConstKind<'_>, if cfg!(bootstrap) { 40 } else { 32 }); impl<'tcx> ConstKind<'tcx> { #[inline] diff --git a/src/librustc_mir_build/hair/mod.rs b/src/librustc_mir_build/hair/mod.rs index 601e4412512ab..3faf730945f53 100644 --- a/src/librustc_mir_build/hair/mod.rs +++ b/src/librustc_mir_build/hair/mod.rs @@ -95,7 +95,7 @@ crate enum StmtKind<'tcx> { // `Expr` is used a lot. Make sure it doesn't unintentionally get bigger. #[cfg(target_arch = "x86_64")] -rustc_data_structures::static_assert_size!(Expr<'_>, 168); +rustc_data_structures::static_assert_size!(Expr<'_>, if cfg!(bootstrap) { 168 } else { 160 }); /// The Hair trait implementor lowers their expressions (`&'tcx H::Expr`) /// into instances of this `Expr` enum. This lowering can be done diff --git a/src/test/ui/print_type_sizes/niche-filling.stdout b/src/test/ui/print_type_sizes/niche-filling.stdout index 301edc0d086b1..1894cd218ee34 100644 --- a/src/test/ui/print_type_sizes/niche-filling.stdout +++ b/src/test/ui/print_type_sizes/niche-filling.stdout @@ -8,12 +8,12 @@ print-type-size variant `Some`: 12 bytes print-type-size field `.0`: 12 bytes print-type-size variant `None`: 0 bytes print-type-size type: `EmbeddedDiscr`: 8 bytes, alignment: 4 bytes +print-type-size discriminant: 1 bytes print-type-size variant `Record`: 7 bytes -print-type-size field `.val`: 4 bytes -print-type-size field `.post`: 2 bytes print-type-size field `.pre`: 1 bytes +print-type-size field `.post`: 2 bytes +print-type-size field `.val`: 4 bytes print-type-size variant `None`: 0 bytes -print-type-size end padding: 1 bytes print-type-size type: `MyOption<Union1<std::num::NonZeroU32>>`: 8 bytes, alignment: 4 bytes print-type-size discriminant: 4 bytes print-type-size variant `Some`: 4 bytes diff --git a/src/test/ui/print_type_sizes/padding.stdout b/src/test/ui/print_type_sizes/padding.stdout index 358033a7ccc9e..3a5fdb7398d6b 100644 --- a/src/test/ui/print_type_sizes/padding.stdout +++ b/src/test/ui/print_type_sizes/padding.stdout @@ -1,21 +1,19 @@ -print-type-size type: `E1`: 12 bytes, alignment: 4 bytes -print-type-size discriminant: 1 bytes -print-type-size variant `B`: 11 bytes -print-type-size padding: 3 bytes -print-type-size field `.0`: 8 bytes, alignment: 4 bytes -print-type-size variant `A`: 7 bytes -print-type-size field `.1`: 1 bytes +print-type-size type: `E1`: 8 bytes, alignment: 4 bytes +print-type-size variant `A`: 8 bytes +print-type-size padding: 1 bytes +print-type-size field `.1`: 1 bytes, alignment: 1 bytes print-type-size padding: 2 bytes print-type-size field `.0`: 4 bytes, alignment: 4 bytes -print-type-size type: `E2`: 12 bytes, alignment: 4 bytes -print-type-size discriminant: 1 bytes -print-type-size variant `B`: 11 bytes -print-type-size padding: 3 bytes -print-type-size field `.0`: 8 bytes, alignment: 4 bytes -print-type-size variant `A`: 7 bytes -print-type-size field `.0`: 1 bytes +print-type-size variant `B`: 8 bytes +print-type-size field `.0`: 8 bytes +print-type-size type: `E2`: 8 bytes, alignment: 4 bytes +print-type-size variant `A`: 8 bytes +print-type-size padding: 1 bytes +print-type-size field `.0`: 1 bytes, alignment: 1 bytes print-type-size padding: 2 bytes print-type-size field `.1`: 4 bytes, alignment: 4 bytes +print-type-size variant `B`: 8 bytes +print-type-size field `.0`: 8 bytes print-type-size type: `S`: 8 bytes, alignment: 4 bytes print-type-size field `.b`: 1 bytes print-type-size field `.a`: 1 bytes diff --git a/src/test/ui/type-sizes.rs b/src/test/ui/type-sizes.rs index 45a702653f506..f419b69f776ce 100644 --- a/src/test/ui/type-sizes.rs +++ b/src/test/ui/type-sizes.rs @@ -102,6 +102,16 @@ enum Option2<A, B> { None } +struct BoolInTheMiddle(std::num::NonZeroU16, bool, u8); + +enum NicheWithData { + A, + B([u16; 5]), + Largest { a1: u32, a2: BoolInTheMiddle, a3: u32 }, + C, + D(u32, u32), +} + pub fn main() { assert_eq!(size_of::<u8>(), 1 as usize); assert_eq!(size_of::<u32>(), 4 as usize); @@ -149,4 +159,12 @@ pub fn main() { struct S1{ a: u16, b: std::num::NonZeroU16, c: u16, d: u8, e: u32, f: u64, g:[u8;2] } assert_eq!(size_of::<S1>(), 24); assert_eq!(size_of::<Option<S1>>(), 24); + + assert_eq!(size_of::<NicheWithData>(), 12); + assert_eq!(size_of::<Option<NicheWithData>>(), 12); + assert_eq!(size_of::<Option<Option<NicheWithData>>>(), 12); + assert_eq!( + size_of::<Option<Option2<&(), Option<NicheWithData>>>>(), + size_of::<(&(), NicheWithData)>() + ); } From 823d5b70a987d61f053acfcebc50904be65ab379 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <ogoffart@woboq.com> Date: Sat, 28 Mar 2020 12:34:50 +0100 Subject: [PATCH 4/7] Add test from #63866 --- src/test/ui/type-sizes.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/ui/type-sizes.rs b/src/test/ui/type-sizes.rs index f419b69f776ce..414d62782976d 100644 --- a/src/test/ui/type-sizes.rs +++ b/src/test/ui/type-sizes.rs @@ -167,4 +167,9 @@ pub fn main() { size_of::<Option<Option2<&(), Option<NicheWithData>>>>(), size_of::<(&(), NicheWithData)>() ); + + pub enum FillPadding { A(std::num::NonZeroU8, u32), B } + assert_eq!(size_of::<FillPadding>(), 8); + assert_eq!(size_of::<Option<FillPadding>>(), 8); + assert_eq!(size_of::<Option<Option<FillPadding>>>(), 8); } From 3ff98d970d03b85ab8b421f44eb854d7e22ce7d9 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <ogoffart@woboq.com> Date: Thu, 2 Apr 2020 12:53:36 +0200 Subject: [PATCH 5/7] Only use the padding for the discriminent if it is bigger than the biggest niche --- src/librustc_middle/ty/layout.rs | 31 +++++++++++++++++--------- src/test/ui/type-sizes.rs | 38 ++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/src/librustc_middle/ty/layout.rs b/src/librustc_middle/ty/layout.rs index 1c400d0bbfadd..d895c6f05ff36 100644 --- a/src/librustc_middle/ty/layout.rs +++ b/src/librustc_middle/ty/layout.rs @@ -900,6 +900,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { let mut niche_variants = VariantIdx::MAX..=VariantIdx::new(0); let mut max_size = Size::ZERO; let mut second_max_size = Size::ZERO; + let mut align = dl.aggregate_align; // The size computations below assume that the padding is minimum. // This is the case when fields are re-ordered. @@ -909,7 +910,6 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { niche_variants = *niche_variants.start().min(&d)..=*niche_variants.end().max(&d); }; - let mut align = dl.aggregate_align; // Find the largest and second largest variant. for (v, fields) in variants.iter_enumerated() { @@ -939,10 +939,6 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { extend_niche_range(v); } } - if !max_size.is_aligned(align.abi) { - // Don't perform niche optimisation if there is padding anyway - dataful_variant = None; - } } else { // Find one non-ZST variant. 'variants: for (v, fields) in variants.iter_enumerated() { @@ -980,12 +976,22 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { .filter_map(|(j, &field)| Some((j, field.largest_niche.as_ref()?))) .max_by_key(|(_, n)| (n.available(dl), cmp::Reverse(n.offset))) .and_then(|(field_index, niche)| { - // make sure there is enough room for the other variants - if struct_reordering_opt - && max_size - (niche.offset + niche.scalar.value.size(dl)) + if struct_reordering_opt { + // make sure there is enough room for the other variants + if max_size - (niche.offset + niche.scalar.value.size(dl)) < second_max_size - { - return None; + { + return None; + } + // Don't perform niche optimisation if there is more padding than + // what's available in the biggest niche + let padding_bits = + (max_size.align_to(align.abi) - max_size).bits(); + if padding_bits >= 128 + || (1 << padding_bits) > niche.available(dl) + { + return None; + } } Some((field_index, niche, niche.reserve(self, count)?)) }); @@ -1028,7 +1034,10 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { } else { st[dataful_variant].fields.offset(field_index) + niche.offset }; - let size = st[dataful_variant].size; + let size = st[dataful_variant].size.align_to(align.abi); + debug_assert!( + !struct_reordering_opt || size == max_size.align_to(align.abi) + ); debug_assert!(st.iter().all(|v| { v.size <= size })); let abi = if st.iter().all(|v| v.abi.is_uninhabited()) { diff --git a/src/test/ui/type-sizes.rs b/src/test/ui/type-sizes.rs index 414d62782976d..a8b7bacb58ffa 100644 --- a/src/test/ui/type-sizes.rs +++ b/src/test/ui/type-sizes.rs @@ -112,6 +112,34 @@ enum NicheWithData { D(u32, u32), } +// A type with almost 2^16 invalid values. +#[repr(u16)] +pub enum NicheU16 { + _0, +} + +pub enum EnumManyVariant<X> { + Dataful(u8, X), + + // 0x100 niche variants. + _00, _01, _02, _03, _04, _05, _06, _07, _08, _09, _0A, _0B, _0C, _0D, _0E, _0F, + _10, _11, _12, _13, _14, _15, _16, _17, _18, _19, _1A, _1B, _1C, _1D, _1E, _1F, + _20, _21, _22, _23, _24, _25, _26, _27, _28, _29, _2A, _2B, _2C, _2D, _2E, _2F, + _30, _31, _32, _33, _34, _35, _36, _37, _38, _39, _3A, _3B, _3C, _3D, _3E, _3F, + _40, _41, _42, _43, _44, _45, _46, _47, _48, _49, _4A, _4B, _4C, _4D, _4E, _4F, + _50, _51, _52, _53, _54, _55, _56, _57, _58, _59, _5A, _5B, _5C, _5D, _5E, _5F, + _60, _61, _62, _63, _64, _65, _66, _67, _68, _69, _6A, _6B, _6C, _6D, _6E, _6F, + _70, _71, _72, _73, _74, _75, _76, _77, _78, _79, _7A, _7B, _7C, _7D, _7E, _7F, + _80, _81, _82, _83, _84, _85, _86, _87, _88, _89, _8A, _8B, _8C, _8D, _8E, _8F, + _90, _91, _92, _93, _94, _95, _96, _97, _98, _99, _9A, _9B, _9C, _9D, _9E, _9F, + _A0, _A1, _A2, _A3, _A4, _A5, _A6, _A7, _A8, _A9, _AA, _AB, _AC, _AD, _AE, _AF, + _B0, _B1, _B2, _B3, _B4, _B5, _B6, _B7, _B8, _B9, _BA, _BB, _BC, _BD, _BE, _BF, + _C0, _C1, _C2, _C3, _C4, _C5, _C6, _C7, _C8, _C9, _CA, _CB, _CC, _CD, _CE, _CF, + _D0, _D1, _D2, _D3, _D4, _D5, _D6, _D7, _D8, _D9, _DA, _DB, _DC, _DD, _DE, _DF, + _E0, _E1, _E2, _E3, _E4, _E5, _E6, _E7, _E8, _E9, _EA, _EB, _EC, _ED, _EE, _EF, + _F0, _F1, _F2, _F3, _F4, _F5, _F6, _F7, _F8, _F9, _FA, _FB, _FC, _FD, _FE, _FF, +} + pub fn main() { assert_eq!(size_of::<u8>(), 1 as usize); assert_eq!(size_of::<u32>(), 4 as usize); @@ -172,4 +200,14 @@ pub fn main() { assert_eq!(size_of::<FillPadding>(), 8); assert_eq!(size_of::<Option<FillPadding>>(), 8); assert_eq!(size_of::<Option<Option<FillPadding>>>(), 8); + + assert_eq!(size_of::<Result<(std::num::NonZeroU8, u8, u8), u16>>(), 4); + assert_eq!(size_of::<Option<Result<(std::num::NonZeroU8, u8, u8), u16>>>(), 4); + assert_eq!(size_of::<Result<(std::num::NonZeroU8, u8, u8, u8), u16>>(), 4); + + assert_eq!(size_of::<EnumManyVariant<u16>>(), 6); + assert_eq!(size_of::<EnumManyVariant<NicheU16>>(), 4); + assert_eq!(size_of::<EnumManyVariant<Option<NicheU16>>>(), 4); + assert_eq!(size_of::<EnumManyVariant<Option2<NicheU16,u8>>>(), 6); + assert_eq!(size_of::<EnumManyVariant<Option<(NicheU16,u8)>>>(), 6); } From 39e9dae9d634298b00d9b0a2e5944b484f4f34a3 Mon Sep 17 00:00:00 2001 From: Olivier Goffart <ogoffart@woboq.com> Date: Sun, 5 Apr 2020 16:01:32 +0200 Subject: [PATCH 6/7] Simplify the detection of the largest variant Since we do not care much about the case where we can use the niche, but that the fields cannot be re-ordered --- src/librustc_middle/ty/layout.rs | 82 ++++++++++++-------------------- 1 file changed, 31 insertions(+), 51 deletions(-) diff --git a/src/librustc_middle/ty/layout.rs b/src/librustc_middle/ty/layout.rs index d895c6f05ff36..8a3e9386ce2b5 100644 --- a/src/librustc_middle/ty/layout.rs +++ b/src/librustc_middle/ty/layout.rs @@ -905,58 +905,38 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { // The size computations below assume that the padding is minimum. // This is the case when fields are re-ordered. let struct_reordering_opt = !def.repr.inhibit_struct_field_reordering_opt(); - if struct_reordering_opt { - let mut extend_niche_range = |d| { - niche_variants = - *niche_variants.start().min(&d)..=*niche_variants.end().max(&d); - }; - // Find the largest and second largest variant. - for (v, fields) in variants.iter_enumerated() { - if absent(fields) { - continue; - } - let mut size = Size::ZERO; - for &f in fields { - align = align.max(f.align); - size += f.size; - } - if size > max_size { - second_max_size = max_size; - max_size = size; - if let Some(d) = dataful_variant { - extend_niche_range(d); - } - dataful_variant = Some(v); - } else if size == max_size { - if let Some(d) = dataful_variant { - extend_niche_range(d); - } - dataful_variant = None; - extend_niche_range(v); - } else { - second_max_size = second_max_size.max(size); - extend_niche_range(v); - } + let mut extend_niche_range = |d| { + niche_variants = + *niche_variants.start().min(&d)..=*niche_variants.end().max(&d); + }; + + // Find the largest and second largest variant. + for (v, fields) in variants.iter_enumerated() { + if absent(fields) { + continue; } - } else { - // Find one non-ZST variant. - 'variants: for (v, fields) in variants.iter_enumerated() { - if absent(fields) { - continue 'variants; + let mut size = Size::ZERO; + for &f in fields { + align = align.max(f.align); + size += f.size; + } + if size > max_size { + second_max_size = max_size; + max_size = size; + if let Some(d) = dataful_variant { + extend_niche_range(d); } - for f in fields { - if !f.is_zst() { - if dataful_variant.is_none() { - dataful_variant = Some(v); - continue 'variants; - } else { - dataful_variant = None; - break 'variants; - } - } - niche_variants = *niche_variants.start().min(&v)..=v + dataful_variant = Some(v); + } else if size == max_size { + if let Some(d) = dataful_variant { + extend_niche_range(d); } + dataful_variant = None; + extend_niche_range(v); + } else { + second_max_size = second_max_size.max(size); + extend_niche_range(v); } } @@ -992,6 +972,8 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { { return None; } + } else if second_max_size > Size::ZERO { + return None; } Some((field_index, niche, niche.reserve(self, count)?)) }); @@ -999,7 +981,6 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { if let Some((field_index, niche, (niche_start, niche_scalar))) = niche_candidate { - let mut align = dl.aggregate_align; let prefix = niche.offset + niche.scalar.value.size(dl); let st = variants .iter_enumerated() @@ -1019,8 +1000,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { )?; st.variants = Variants::Single { index: j }; - align = align.max(st.align); - + debug_assert_eq!(align, align.max(st.align)); Ok(st) }) .collect::<Result<IndexVec<VariantIdx, _>, _>>()?; From 0d43af18aea985e5685149c5469337935000637f Mon Sep 17 00:00:00 2001 From: Olivier Goffart <ogoffart@woboq.com> Date: Sat, 11 Apr 2020 12:58:26 +0200 Subject: [PATCH 7/7] Do both the non-niche layout and the niche layout and compare the result Instead of using estimation euristic to find out if we should do one or the other layout, always perform the non-niche layout and, when possible, the niche layout, and compare the resulting layout. In my opinion, this is less efficient, but was requested in the review because it is hard to proof that the estimation is correct in every case. --- src/librustc_middle/ty/layout.rs | 380 +++++++++++++++---------------- 1 file changed, 188 insertions(+), 192 deletions(-) diff --git a/src/librustc_middle/ty/layout.rs b/src/librustc_middle/ty/layout.rs index 8a3e9386ce2b5..6b6278f659b79 100644 --- a/src/librustc_middle/ty/layout.rs +++ b/src/librustc_middle/ty/layout.rs @@ -886,196 +886,6 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { // that allow representation optimization.) assert!(def.is_enum()); - // The current code for niche-filling relies on variant indices - // instead of actual discriminants, so dataful enums with - // explicit discriminants (RFC #2363) would misbehave. - let no_explicit_discriminants = def - .variants - .iter_enumerated() - .all(|(i, v)| v.discr == ty::VariantDiscr::Relative(i.as_u32())); - - // Niche-filling enum optimization. - if !def.repr.inhibit_enum_layout_opt() && no_explicit_discriminants { - let mut dataful_variant = None; - let mut niche_variants = VariantIdx::MAX..=VariantIdx::new(0); - let mut max_size = Size::ZERO; - let mut second_max_size = Size::ZERO; - let mut align = dl.aggregate_align; - - // The size computations below assume that the padding is minimum. - // This is the case when fields are re-ordered. - let struct_reordering_opt = !def.repr.inhibit_struct_field_reordering_opt(); - - let mut extend_niche_range = |d| { - niche_variants = - *niche_variants.start().min(&d)..=*niche_variants.end().max(&d); - }; - - // Find the largest and second largest variant. - for (v, fields) in variants.iter_enumerated() { - if absent(fields) { - continue; - } - let mut size = Size::ZERO; - for &f in fields { - align = align.max(f.align); - size += f.size; - } - if size > max_size { - second_max_size = max_size; - max_size = size; - if let Some(d) = dataful_variant { - extend_niche_range(d); - } - dataful_variant = Some(v); - } else if size == max_size { - if let Some(d) = dataful_variant { - extend_niche_range(d); - } - dataful_variant = None; - extend_niche_range(v); - } else { - second_max_size = second_max_size.max(size); - extend_niche_range(v); - } - } - - if niche_variants.start() > niche_variants.end() { - dataful_variant = None; - } - - if let Some(dataful_variant) = dataful_variant { - let count = (niche_variants.end().as_u32() - - niche_variants.start().as_u32() - + 1) as u128; - - // Find the field with the largest niche - let niche_candidate = variants[dataful_variant] - .iter() - .enumerate() - .filter_map(|(j, &field)| Some((j, field.largest_niche.as_ref()?))) - .max_by_key(|(_, n)| (n.available(dl), cmp::Reverse(n.offset))) - .and_then(|(field_index, niche)| { - if struct_reordering_opt { - // make sure there is enough room for the other variants - if max_size - (niche.offset + niche.scalar.value.size(dl)) - < second_max_size - { - return None; - } - // Don't perform niche optimisation if there is more padding than - // what's available in the biggest niche - let padding_bits = - (max_size.align_to(align.abi) - max_size).bits(); - if padding_bits >= 128 - || (1 << padding_bits) > niche.available(dl) - { - return None; - } - } else if second_max_size > Size::ZERO { - return None; - } - Some((field_index, niche, niche.reserve(self, count)?)) - }); - - if let Some((field_index, niche, (niche_start, niche_scalar))) = - niche_candidate - { - let prefix = niche.offset + niche.scalar.value.size(dl); - let st = variants - .iter_enumerated() - .map(|(j, v)| { - let mut st = self.univariant_uninterned( - ty, - v, - &def.repr, - if j == dataful_variant || second_max_size == Size::ZERO { - StructKind::AlwaysSized - } else { - StructKind::Prefixed( - prefix, - Align::from_bytes(1).unwrap(), - ) - }, - )?; - st.variants = Variants::Single { index: j }; - - debug_assert_eq!(align, align.max(st.align)); - Ok(st) - }) - .collect::<Result<IndexVec<VariantIdx, _>, _>>()?; - - let niche_offset = if struct_reordering_opt { - debug_assert_eq!( - st[dataful_variant].fields.offset(field_index), - Size::ZERO - ); - niche.offset - } else { - st[dataful_variant].fields.offset(field_index) + niche.offset - }; - let size = st[dataful_variant].size.align_to(align.abi); - debug_assert!( - !struct_reordering_opt || size == max_size.align_to(align.abi) - ); - debug_assert!(st.iter().all(|v| { v.size <= size })); - - let abi = if st.iter().all(|v| v.abi.is_uninhabited()) { - Abi::Uninhabited - } else if second_max_size == Size::ZERO { - match st[dataful_variant].abi { - Abi::Scalar(_) => Abi::Scalar(niche_scalar.clone()), - Abi::ScalarPair(ref first, ref second) => { - // We need to use scalar_unit to reset the - // valid range to the maximal one for that - // primitive, because only the niche is - // guaranteed to be initialised, not the - // other primitive. - if niche_offset.bytes() == 0 { - Abi::ScalarPair( - niche_scalar.clone(), - scalar_unit(second.value), - ) - } else { - Abi::ScalarPair( - scalar_unit(first.value), - niche_scalar.clone(), - ) - } - } - _ => Abi::Aggregate { sized: true }, - } - } else { - Abi::Aggregate { sized: true } - }; - - let largest_niche = - Niche::from_scalar(dl, niche_offset, niche_scalar.clone()); - - return Ok(tcx.intern_layout(Layout { - variants: Variants::Multiple { - discr: niche_scalar, - discr_kind: DiscriminantKind::Niche { - dataful_variant, - niche_variants, - niche_start, - }, - discr_index: 0, - variants: st, - }, - fields: FieldsShape::Arbitrary { - offsets: vec![niche_offset], - memory_index: vec![0], - }, - abi, - largest_niche, - size, - align, - })); - } - } - } - let (mut min, mut max) = (i128::MAX, i128::MIN); let discr_type = def.repr.discr_type(); let bits = Integer::from_attr(self, discr_type).size().bits(); @@ -1226,6 +1036,194 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { value: Int(ity, signed), valid_range: (min as u128 & tag_mask)..=(max as u128 & tag_mask), }; + let largest_niche = Niche::from_scalar(dl, Size::ZERO, tag.clone()); + + // The current code for niche-filling relies on variant indices + // instead of actual discriminants, so dataful enums with + // explicit discriminants (RFC #2363) would misbehave. + let no_explicit_discriminants = def + .variants + .iter_enumerated() + .all(|(i, v)| v.discr == ty::VariantDiscr::Relative(i.as_u32())); + + // Niche-filling enum optimization. + if !def.repr.inhibit_enum_layout_opt() && no_explicit_discriminants { + let mut dataful_variant = None; + let mut niche_variants = VariantIdx::MAX..=VariantIdx::new(0); + let mut max_size = Size::ZERO; + let mut second_max_size = Size::ZERO; + let mut align = dl.aggregate_align; + + // The size computations below assume that the padding is minimum. + // This is the case when fields are re-ordered. + let struct_reordering_opt = !def.repr.inhibit_struct_field_reordering_opt(); + + let mut extend_niche_range = |d| { + niche_variants = + *niche_variants.start().min(&d)..=*niche_variants.end().max(&d); + }; + + // Find the largest and second largest variant. + for (v, fields) in variants.iter_enumerated() { + if absent(fields) { + continue; + } + let mut size = Size::ZERO; + for &f in fields { + align = align.max(f.align); + size += f.size; + } + if size > max_size { + second_max_size = max_size; + max_size = size; + if let Some(d) = dataful_variant { + extend_niche_range(d); + } + dataful_variant = Some(v); + } else if size == max_size { + if let Some(d) = dataful_variant { + extend_niche_range(d); + } + dataful_variant = None; + extend_niche_range(v); + } else { + second_max_size = second_max_size.max(size); + extend_niche_range(v); + } + } + + if niche_variants.start() > niche_variants.end() { + dataful_variant = None; + } + + if let Some(dataful_variant) = dataful_variant { + let count = (niche_variants.end().as_u32() + - niche_variants.start().as_u32() + + 1) as u128; + + // Find the field with the largest niche + let niche_candidate = variants[dataful_variant] + .iter() + .enumerate() + .filter_map(|(j, &field)| Some((j, field.largest_niche.as_ref()?))) + .max_by_key(|(_, n)| (n.available(dl), cmp::Reverse(n.offset))) + .and_then(|(field_index, niche)| { + if !struct_reordering_opt && second_max_size > Size::ZERO { + return None; + } + // make sure there is enough room for the other variants + if max_size - (niche.offset + niche.scalar.value.size(dl)) + < second_max_size + { + return None; + } + Some((field_index, niche, niche.reserve(self, count)?)) + }); + + if let Some((field_index, niche, (niche_start, niche_scalar))) = + niche_candidate + { + let prefix = niche.offset + niche.scalar.value.size(dl); + let st = variants + .iter_enumerated() + .map(|(j, v)| { + let mut st = self.univariant_uninterned( + ty, + v, + &def.repr, + if j == dataful_variant || second_max_size == Size::ZERO { + StructKind::AlwaysSized + } else { + StructKind::Prefixed( + prefix, + Align::from_bytes(1).unwrap(), + ) + }, + )?; + st.variants = Variants::Single { index: j }; + + debug_assert_eq!(align, align.max(st.align)); + Ok(st) + }) + .collect::<Result<IndexVec<VariantIdx, _>, _>>()?; + + let niche_offset = if struct_reordering_opt { + debug_assert_eq!( + st[dataful_variant].fields.offset(field_index), + Size::ZERO + ); + niche.offset + } else { + st[dataful_variant].fields.offset(field_index) + niche.offset + }; + + let new_size = st[dataful_variant].size.align_to(align.abi); + debug_assert!( + !struct_reordering_opt || new_size == max_size.align_to(align.abi) + ); + debug_assert!(st.iter().all(|v| { v.size <= new_size })); + + let new_largest_niche = + Niche::from_scalar(dl, niche_offset, niche_scalar.clone()); + + if new_size < size + || new_largest_niche.as_ref().map_or(0, |n| n.available(dl)) + > largest_niche.as_ref().map_or(0, |n| n.available(dl)) + { + let abi = if st.iter().all(|v| v.abi.is_uninhabited()) { + Abi::Uninhabited + } else if second_max_size == Size::ZERO { + match st[dataful_variant].abi { + Abi::Scalar(_) => Abi::Scalar(niche_scalar.clone()), + Abi::ScalarPair(ref first, ref second) => { + // We need to use scalar_unit to reset the + // valid range to the maximal one for that + // primitive, because only the niche is + // guaranteed to be initialised, not the + // other primitive. + if niche_offset.bytes() == 0 { + Abi::ScalarPair( + niche_scalar.clone(), + scalar_unit(second.value), + ) + } else { + Abi::ScalarPair( + scalar_unit(first.value), + niche_scalar.clone(), + ) + } + } + _ => Abi::Aggregate { sized: true }, + } + } else { + Abi::Aggregate { sized: true } + }; + + return Ok(tcx.intern_layout(Layout { + variants: Variants::Multiple { + discr: niche_scalar, + discr_kind: DiscriminantKind::Niche { + dataful_variant, + niche_variants, + niche_start, + }, + discr_index: 0, + variants: st, + }, + fields: FieldsShape::Arbitrary { + offsets: vec![niche_offset], + memory_index: vec![0], + }, + abi, + largest_niche: new_largest_niche, + size: new_size, + align, + })); + } + } + } + } + let mut abi = Abi::Aggregate { sized: true }; if tag.value.size(dl) == size { abi = Abi::Scalar(tag.clone()); @@ -1292,8 +1290,6 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { abi = Abi::Uninhabited; } - let largest_niche = Niche::from_scalar(dl, Size::ZERO, tag.clone()); - tcx.intern_layout(Layout { variants: Variants::Multiple { discr: tag,