Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

layout: Always use the largest tag size that fits. #63902

Closed
wants to merge 2 commits into from
Closed

layout: Always use the largest tag size that fits. #63902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ac9003
layout: Factor out a utility function.
hvenev Aug 25, 2019
efc446f
layout: Always use the largest tag size that fits.
hvenev Aug 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
223 changes: 97 additions & 126 deletions src/librustc/ty/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,33 +267,61 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
}
}

fn align_and_pack(&self, repr: &ReprOptions) -> (AbiAndPrefAlign, Option<Align>) {
let dl = self.data_layout();
if repr.packed() {
let pack = Align::from_bytes(repr.pack as u64).unwrap();
if repr.align > 0 {
bug!("adt cannot be packed and aligned");
}
(dl.i8_align, Some(pack))
} else {
let align = Align::from_bytes(repr.align as u64).unwrap();
(dl.aggregate_align.max(AbiAndPrefAlign::new(align)), None)
}
}

fn variant_size(&self,
fields: &[TyLayout<'_>],
repr: &ReprOptions) -> Option<(Size, AbiAndPrefAlign)> {
let dl = self.data_layout();
let (mut align, pack) = self.align_and_pack(repr);

let optimize = !repr.inhibit_struct_field_reordering_opt();

let mut size = Size::ZERO;
for field in fields.iter() {
assert!(!field.is_unsized());
let field_align = if let Some(pack) = pack {
field.align.min(AbiAndPrefAlign::new(pack))
} else {
field.align
};
if !optimize {
size = size.align_to(field_align.abi);
}
align = align.max(field_align);
size = size.checked_add(field.size, dl)?;
}
if !optimize {
size = size.align_to(align.abi);
}
Some((size, align))
}

fn univariant_uninterned(&self,
ty: Ty<'tcx>,
fields: &[TyLayout<'_>],
repr: &ReprOptions,
kind: StructKind) -> Result<LayoutDetails, LayoutError<'tcx>> {
let dl = self.data_layout();
let packed = repr.packed();
if packed && repr.align > 0 {
bug!("struct cannot be packed and aligned");
}

let pack = Align::from_bytes(repr.pack as u64).unwrap();

let mut align = if packed {
dl.i8_align
} else {
dl.aggregate_align
};
let (mut align, pack) = self.align_and_pack(repr);

let mut sized = true;
let mut offsets = vec![Size::ZERO; fields.len()];
let mut inverse_memory_index: Vec<u32> = (0..fields.len() as u32).collect();

let mut optimize = !repr.inhibit_struct_field_reordering_opt();
if let StructKind::Prefixed(_, align) = kind {
optimize &= align.bytes() == 1;
}
let optimize = !repr.inhibit_struct_field_reordering_opt();

if optimize {
let end = if let StructKind::MaybeUnsized = kind {
Expand All @@ -303,7 +331,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
};
let optimizing = &mut inverse_memory_index[..end];
let field_align = |f: &TyLayout<'_>| {
if packed { f.align.abi.min(pack) } else { f.align.abi }
if let Some(pack) = pack { f.align.abi.min(pack) } else { f.align.abi }
};
match kind {
StructKind::AlwaysSized |
Expand Down Expand Up @@ -334,7 +362,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
let mut largest_niche_available = 0;

if let StructKind::Prefixed(prefix_size, prefix_align) = kind {
let prefix_align = if packed {
let prefix_align = if let Some(pack) = pack {
prefix_align.min(pack)
} else {
prefix_align
Expand All @@ -355,7 +383,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
}

// Invariant: offset < dl.obj_size_bound() <= 1<<61
let field_align = if packed {
let field_align = if let Some(pack) = pack {
field.align.min(AbiAndPrefAlign::new(pack))
} else {
field.align
Expand All @@ -379,12 +407,6 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
.ok_or(LayoutError::SizeOverflow(ty))?;
}

if repr.align > 0 {
let repr_align = repr.align as u64;
align = align.max(AbiAndPrefAlign::new(Align::from_bytes(repr_align).unwrap()));
debug!("univariant repr_align: {:?}", repr_align);
}

debug!("univariant min_size: {:?}", offset);
let min_size = offset;

Expand Down Expand Up @@ -730,24 +752,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
}).collect::<Result<IndexVec<VariantIdx, _>, _>>()?;

if def.is_union() {
let packed = def.repr.packed();
if packed && def.repr.align > 0 {
bug!("Union cannot be packed and aligned");
}

let pack = Align::from_bytes(def.repr.pack as u64).unwrap();

let mut align = if packed {
dl.i8_align
} else {
dl.aggregate_align
};

if def.repr.align > 0 {
let repr_align = def.repr.align as u64;
align = align.max(
AbiAndPrefAlign::new(Align::from_bytes(repr_align).unwrap()));
}
let (mut align, pack) = self.align_and_pack(&def.repr);

let optimize = !def.repr.inhibit_union_abi_opt();
let mut size = Size::ZERO;
Expand All @@ -756,7 +761,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
for field in &variants[index] {
assert!(!field.is_unsized());

let field_align = if packed {
let field_align = if let Some(pack) = pack {
field.align.min(AbiAndPrefAlign::new(pack))
} else {
field.align
Expand Down Expand Up @@ -906,6 +911,16 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
return Ok(tcx.intern_layout(st));
}

let mut align = dl.i8_align;
let mut max_size = Size::ZERO;

for fields in variants.iter() {
let (v_size, v_align) = self.variant_size(fields, &def.repr)
.ok_or(LayoutError::SizeOverflow(ty))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it could be expensive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also doesn't take field sorting and the offset for the tag into account?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It considers field sorting, but it does not compute it. If sorting happens, all padding can be at the start.

This also means it's relatively cheap.

max_size = max_size.max(v_size);
align = align.max(v_align);
}

// The current code for niche-filling relies on variant indices
// instead of actual discriminants, so dataful enums with
// explicit discriminants (RFC #2363) would misbehave.
Expand Down Expand Up @@ -956,14 +971,11 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
None => continue,
};

let mut align = dl.aggregate_align;
let st = variants.iter_enumerated().map(|(j, v)| {
let mut st = self.univariant_uninterned(ty, v,
&def.repr, StructKind::AlwaysSized)?;
st.variants = Variants::Single { index: j };

align = align.max(st.align);

assert!(st.align.abi <= align.abi && st.align.pref <= align.pref);
Ok(st)
}).collect::<Result<IndexVec<VariantIdx, _>, _>>()?;

Expand Down Expand Up @@ -1025,6 +1037,8 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
}
}

let mut gap = max_size.align_to(align.abi) - max_size;

let (mut min, mut max) = (i128::max_value(), i128::min_value());
let discr_type = def.repr.discr_type();
let bits = Integer::from_attr(self, discr_type).size().bits();
Expand All @@ -1048,52 +1062,6 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
assert!(min <= max, "discriminant range is {}...{}", min, max);
let (min_ity, signed) = Integer::repr_discr(tcx, ty, &def.repr, min, max);

let mut align = dl.aggregate_align;
let mut size = Size::ZERO;

// We're interested in the smallest alignment, so start large.
let mut start_align = Align::from_bytes(256).unwrap();
assert_eq!(Integer::for_align(dl, start_align), None);

// repr(C) on an enum tells us to make a (tag, union) layout,
// so we need to grow the prefix alignment to be at least
// the alignment of the union. (This value is used both for
// determining the alignment of the overall enum, and the
// determining the alignment of the payload after the tag.)
let mut prefix_align = min_ity.align(dl).abi;
if def.repr.c() {
for fields in &variants {
for field in fields {
prefix_align = prefix_align.max(field.align.abi);
}
}
}

// Create the set of structs that represent each variant.
let mut layout_variants = variants.iter_enumerated().map(|(i, field_layouts)| {
let mut st = self.univariant_uninterned(ty, &field_layouts,
&def.repr, StructKind::Prefixed(min_ity.size(), prefix_align))?;
st.variants = Variants::Single { index: i };
// Find the first field we can't move later
// to make room for a larger discriminant.
for field in st.fields.index_by_increasing_offset().map(|j| field_layouts[j]) {
if !field.is_zst() || field.align.abi.bytes() != 1 {
start_align = start_align.min(field.align.abi);
break;
}
}
size = cmp::max(size, st.size);
align = align.max(st.align);
Ok(st)
}).collect::<Result<IndexVec<VariantIdx, _>, _>>()?;

// Align the maximum variant size to the largest alignment.
size = size.align_to(align.abi);

if size.bytes() >= dl.obj_size_bound() {
return Err(LayoutError::SizeOverflow(ty));
}

let typeck_ity = Integer::from_attr(dl, def.repr.discr_type());
if typeck_ity < min_ity {
// It is a bug if Layout decided on a greater discriminant size than typeck for
Expand All @@ -1111,47 +1079,50 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
// after this point – we’ll just truncate the value we load in codegen.
}

// 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.
if min_ity.size() > gap {
gap += (min_ity.size() - gap).align_to(align.abi);
}

// 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.

// Use the initial field alignment
let mut ity = if def.repr.c() || def.repr.int.is_some() {
let ity = if def.repr.inhibit_enum_layout_opt() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a good idea, the old condition here is not about optimizations, it's about whether the discriminant is being forced with #[repr].

min_ity
} else {
Integer::for_align(dl, start_align).unwrap_or(min_ity)
Integer::approximate_size(gap).unwrap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pick something large enough to increase the alignment, you'll easily waste space in types containing this one.
If you think this will never happen, that's still not obvious from the code, and you'd have to either compute both and assert they're the same, and we can land that, crater it, etc. or prove it formally somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let gap0 be the original value of gap. We either have gap = gap0 < align.abi, or gap = gap0 + (min_ity.size() - gap0).align_to(align.abi) < gap0 + min_ity.size() - gap0 + align.abi = min_ity.size() + align.abi.

ity.size() <= gap. In both cases ity.size() <= gap < min_ity.size() + align.abi, therefore either ity = min_ity or ity.size() < align.abi.

};

// If the alignment is not larger than the chosen discriminant size,
// don't use the alignment as the final size.
if ity <= min_ity {
ity = min_ity;
} else {
// Patch up the variants' first few fields.
let old_ity_size = min_ity.size();
let new_ity_size = ity.size();
for variant in &mut layout_variants {
match variant.fields {
FieldPlacement::Arbitrary { ref mut offsets, .. } => {
for i in offsets {
if *i <= old_ity_size {
assert_eq!(*i, old_ity_size);
*i = new_ity_size;
}
}
// We might be making the struct larger.
if variant.size <= old_ity_size {
variant.size = new_ity_size;
}
}
_ => bug!()
}
}
let mut prefix_align = ity.align(dl).abi;
let align = align.max(AbiAndPrefAlign::new(prefix_align));

// repr(C) on an enum tells us to make a (tag, union) layout,
// so we need to grow the prefix alignment to be at least
// the alignment of the union. (This value is used both for
// determining the alignment of the overall enum, and the
// determining the alignment of the payload after the tag.)
if def.repr.c() {
prefix_align = align.abi;
}

let mut size = Size::ZERO;

// Create the set of structs that represent each variant.
let layout_variants = variants.iter_enumerated().map(|(i, field_layouts)| {
let mut st = self.univariant_uninterned(ty, &field_layouts,
&def.repr, StructKind::Prefixed(ity.size(), prefix_align))?;
st.variants = Variants::Single { index: i };
size = cmp::max(size, st.size);
assert!(st.align.abi <= align.abi && st.align.pref <= align.pref);
Ok(st)
}).collect::<Result<IndexVec<VariantIdx, _>, _>>()?;

// Align the maximum variant size to the largest alignment.
size = size.align_to(align.abi);

if size.bytes() >= dl.obj_size_bound() {
return Err(LayoutError::SizeOverflow(ty));
}

let tag_mask = !0u128 >> (128 - ity.size().bits());
Expand Down
10 changes: 10 additions & 0 deletions src/librustc_target/abi/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,16 @@ impl Integer {
}
I8
}

/// Finds the largest integer with the given size or less.
pub fn approximate_size(wanted: Size) -> Option<Integer> {
for &candidate in &[I64, I32, I16, I8] {
if wanted >= candidate.size() {
return Some(candidate);
}
}
None
}
}


Expand Down
2 changes: 1 addition & 1 deletion src/test/codegen/align-enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub enum Align64 {
A(u32),
B(u32),
}
// CHECK: %Align64 = type { [0 x i32], i32, [15 x i32] }
// CHECK: %Align64 = type { [0 x i64], i64, [7 x i64] }
Copy link
Member

@eddyb eddyb Aug 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're actually moving the field? How do you know that won't cause the field sorting to produce worse results?


pub struct Nested64 {
a: u8,
Expand Down
11 changes: 10 additions & 1 deletion src/test/codegen/align-struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ pub enum Enum64 {
A(Align64),
B(i32),
}
// CHECK: %Enum64 = type { [0 x i32], i32, [31 x i32] }
// CHECK: %Enum64 = type { [0 x i64], i64, [15 x i64] }
// CHECK: %"Enum64::A" = type { [8 x i64], %Align64, [0 x i64] }
// CHECK: %"Enum64::B" = type { [2 x i32], i32, [1 x i32] }

// CHECK-LABEL: @align64
#[no_mangle]
Expand Down Expand Up @@ -71,3 +72,11 @@ pub fn enum64(a: Align64) -> Enum64 {
let e64 = Enum64::A(a);
e64
}

// CHECK-LABEL: @enum64_b
#[no_mangle]
pub fn enum64_b(b: i32) -> Enum64 {
// CHECK: %e64 = alloca %Enum64, align 64
let e64 = Enum64::B(b);
e64
}
Loading