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.

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

struct field reordering and optimization #37429

Merged
merged 28 commits into from
Dec 18, 2016
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
cae94e8
Optimize anything using a layout::Struct by introducing a mapping fro…
ahicks92 Oct 16, 2016
8cfbffe
Fix bugs to optimizing enums:
ahicks92 Nov 16, 2016
1969aeb
Make constant field access account for field reordering.
ahicks92 Nov 18, 2016
2746903
Fix extern-pass-empty test, which needed repr(C)
ahicks92 Nov 18, 2016
d754778
Fix tuple and closure literals.
ahicks92 Nov 20, 2016
c7ec0df
Add yet more missing #[repr(C)] to tests
ahicks92 Nov 20, 2016
0e61c0e
Incorporate a bunch of review comments.
ahicks92 Nov 20, 2016
8e852e9
Struct::new takes a vec, avoiding double allocation in some cases
ahicks92 Nov 20, 2016
3d23dc7
Modify debuginfo to deal with the difference between source and memor…
ahicks92 Nov 21, 2016
adae9bc
Make tidy
ahicks92 Nov 21, 2016
e7c3540
Use an enum to differentiate between kinds of structs.
ahicks92 Nov 21, 2016
1af8e14
First attempt at detecting if structs can ever be unsized
ahicks92 Nov 22, 2016
cb21cc5
Don't optimize pairs
ahicks92 Nov 22, 2016
b3c285f
Fix checking to see if the last field of a struct can be unsized.
ahicks92 Nov 22, 2016
487ef58
Fix type-sizes test
ahicks92 Nov 22, 2016
5adf694
Make tidy
ahicks92 Nov 22, 2016
74f5c61
Change how type-sizes works slightly: we want to ensure that [i16; 0]…
ahicks92 Nov 22, 2016
cf5f80c
Fix having multiple reprs on the same type.
ahicks92 Nov 23, 2016
c8c3579
Fix closure arguments which are immediate because of field reordering.
ahicks92 Nov 23, 2016
052e59c
Make tidy
ahicks92 Nov 23, 2016
e9580e2
Some small fixes to how structs/enums are optimized
ahicks92 Nov 23, 2016
025456e
Incorporate review comments
ahicks92 Nov 23, 2016
cfe1a77
Fix -Z print-type-sizes and tests.
ahicks92 Nov 26, 2016
a65cc1e
Fix error introduced during last rebase
ahicks92 Nov 30, 2016
9966bbd
Fix computation of enum names based off the discrfield in the case of…
ahicks92 Dec 3, 2016
79c35bb
Add a case to type-sizes to explicitly verify that field reordering t…
ahicks92 Dec 7, 2016
f22a22b
Incorporate review comments.
ahicks92 Dec 16, 2016
ff59474
flock needs repr(C)
ahicks92 Dec 16, 2016
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
7 changes: 6 additions & 1 deletion src/librustc/session/code_stats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,12 @@ impl CodeStats {
max_variant_size = cmp::max(max_variant_size, size);

let mut min_offset = discr_size;
for field in fields {

// We want to print fields by increasing offset.
let mut fields = fields.clone();
fields.sort_by_key(|f| f.offset);

for field in fields.iter() {
let FieldInfo { ref name, offset, size, align } = *field;

// Include field alignment in output only if it caused padding injection
Expand Down
430 changes: 307 additions & 123 deletions src/librustc/ty/layout.rs

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion src/librustc_lint/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for VariantSizeDifferences {
if let Layout::General { ref variants, ref size, discr, .. } = *layout {
let discr_size = Primitive::Int(discr).size(&cx.tcx.data_layout).bytes();

debug!("enum `{}` is {} bytes large", t, size.bytes());
debug!("enum `{}` is {} bytes large with layout:\n{:#?}",
t, size.bytes(), layout);

let (largest, slargest, largest_index) = enum_definition.variants
.iter()
Expand Down
53 changes: 28 additions & 25 deletions src/librustc_trans/adt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,14 @@ pub fn finish_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
| layout::UntaggedUnion { .. } | layout::RawNullablePointer { .. } => { }
layout::Univariant { ..}
| layout::StructWrappedNullablePointer { .. } => {
let (nonnull_variant, packed) = match *l {
layout::Univariant { ref variant, .. } => (0, variant.packed),
let (nonnull_variant_index, nonnull_variant, packed) = match *l {
layout::Univariant { ref variant, .. } => (0, variant, variant.packed),
layout::StructWrappedNullablePointer { nndiscr, ref nonnull, .. } =>
(nndiscr, nonnull.packed),
(nndiscr, nonnull, nonnull.packed),
_ => unreachable!()
};
let fields = compute_fields(cx, t, nonnull_variant as usize, true);
llty.set_struct_body(&struct_llfields(cx, &fields, false, false),
let fields = compute_fields(cx, t, nonnull_variant_index as usize, true);
llty.set_struct_body(&struct_llfields(cx, &fields, nonnull_variant, false, false),
packed)
},
_ => bug!("This function cannot handle {} with layout {:#?}", t, l)
Expand Down Expand Up @@ -188,7 +188,7 @@ fn generic_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
let fields = compute_fields(cx, t, nndiscr as usize, false);
match name {
None => {
Type::struct_(cx, &struct_llfields(cx, &fields, sizing, dst),
Type::struct_(cx, &struct_llfields(cx, &fields, nonnull, sizing, dst),
nonnull.packed)
}
Some(name) => {
Expand All @@ -203,7 +203,7 @@ fn generic_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
let fields = compute_fields(cx, t, 0, true);
match name {
None => {
let fields = struct_llfields(cx, &fields, sizing, dst);
let fields = struct_llfields(cx, &fields, &variant, sizing, dst);
Type::struct_(cx, &fields, variant.packed)
}
Some(name) => {
Expand Down Expand Up @@ -291,12 +291,14 @@ fn union_fill(cx: &CrateContext, size: u64, align: u64) -> Type {


fn struct_llfields<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, fields: &Vec<Ty<'tcx>>,
variant: &layout::Struct,
sizing: bool, dst: bool) -> Vec<Type> {
let fields = variant.field_index_by_increasing_offset().map(|i| fields[i as usize]);
if sizing {
fields.iter().filter(|&ty| !dst || type_is_sized(cx.tcx(), *ty))
.map(|&ty| type_of::sizing_type_of(cx, ty)).collect()
fields.filter(|ty| !dst || type_is_sized(cx.tcx(), *ty))
.map(|ty| type_of::sizing_type_of(cx, ty)).collect()
} else {
fields.iter().map(|&ty| type_of::in_memory_type_of(cx, ty)).collect()
fields.map(|ty| type_of::in_memory_type_of(cx, ty)).collect()
}
}

Expand Down Expand Up @@ -564,16 +566,16 @@ pub fn trans_field_ptr_builder<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>,
fn struct_field_ptr<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>,
st: &layout::Struct, fields: &Vec<Ty<'tcx>>, val: MaybeSizedValue,
ix: usize, needs_cast: bool) -> ValueRef {
let ccx = bcx.ccx();
let fty = fields[ix];
let ccx = bcx.ccx();
let ll_fty = type_of::in_memory_type_of(bcx.ccx(), fty);
if bcx.is_unreachable() {
return C_undef(ll_fty.ptr_to());
}

let ptr_val = if needs_cast {
let fields = fields.iter().map(|&ty| {
type_of::in_memory_type_of(ccx, ty)
let fields = st.field_index_by_increasing_offset().map(|i| {
type_of::in_memory_type_of(ccx, fields[i])
}).collect::<Vec<_>>();
let real_ty = Type::struct_(ccx, &fields[..], st.packed);
bcx.pointercast(val.value, real_ty.ptr_to())
Expand All @@ -585,15 +587,15 @@ fn struct_field_ptr<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>,
// * First field - Always aligned properly
// * Packed struct - There is no alignment padding
// * Field is sized - pointer is properly aligned already
if ix == 0 || st.packed || type_is_sized(bcx.tcx(), fty) {
return bcx.struct_gep(ptr_val, ix);
if st.offsets[ix] == layout::Size::from_bytes(0) || st.packed || type_is_sized(bcx.tcx(), fty) {
return bcx.struct_gep(ptr_val, st.memory_index[ix] as usize);
}

// If the type of the last field is [T] or str, then we don't need to do
// any adjusments
match fty.sty {
ty::TySlice(..) | ty::TyStr => {
return bcx.struct_gep(ptr_val, ix);
return bcx.struct_gep(ptr_val, st.memory_index[ix] as usize);
}
_ => ()
}
Expand Down Expand Up @@ -755,8 +757,12 @@ fn build_const_struct<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
// offset of current value
let mut offset = 0;
let mut cfields = Vec::new();
let offsets = st.offsets.iter().map(|i| i.bytes());
for (&val, target_offset) in vals.iter().zip(offsets) {
cfields.reserve(st.offsets.len()*2);

let parts = st.field_index_by_increasing_offset().map(|i| {
(&vals[i], st.offsets[i].bytes())
});
for (&val, target_offset) in parts {
if offset < target_offset {
cfields.push(padding(ccx, target_offset - offset));
offset = target_offset;
Expand Down Expand Up @@ -807,14 +813,11 @@ pub fn const_get_field<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>,
let l = ccx.layout_of(t);
match *l {
layout::CEnum { .. } => bug!("element access in C-like enum const"),
layout::Univariant { .. } | layout::Vector { .. } => const_struct_field(val, ix),
layout::Univariant { ref variant, .. } => {
const_struct_field(val, variant.memory_index[ix] as usize)
}
layout::Vector { .. } => const_struct_field(val, ix),
layout::UntaggedUnion { .. } => const_struct_field(val, 0),
layout::General { .. } => const_struct_field(val, ix + 1),
layout::RawNullablePointer { .. } => {
assert_eq!(ix, 0);
val
},
layout::StructWrappedNullablePointer{ .. } => const_struct_field(val, ix),
_ => bug!("{} does not have fields.", t)
}
}
Expand Down
7 changes: 5 additions & 2 deletions src/librustc_trans/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,9 @@ pub fn alloca(cx: Block, ty: Type, name: &str) -> ValueRef {
}
}
DebugLoc::None.apply(cx.fcx);
Alloca(cx, ty, name)
let result = Alloca(cx, ty, name);
debug!("alloca({:?}) = {:?}", name, result);
result
}

impl<'blk, 'tcx> FunctionContext<'blk, 'tcx> {
Expand Down Expand Up @@ -1868,7 +1870,8 @@ fn gather_type_sizes<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
match **layout {
Layout::StructWrappedNullablePointer { nonnull: ref variant_layout,
nndiscr,
discrfield: _ } => {
discrfield: _,
discrfield_source: _ } => {
debug!("print-type-size t: `{:?}` adt struct-wrapped nullable nndiscr {} is {:?}",
ty, nndiscr, variant_layout);
let variant_def = &adt_def.variants[nndiscr as usize];
Expand Down
55 changes: 33 additions & 22 deletions src/librustc_trans/debuginfo/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -881,25 +881,28 @@ impl<'tcx> MemberDescriptionFactory<'tcx> {

// Creates MemberDescriptions for the fields of a struct
struct StructMemberDescriptionFactory<'tcx> {
ty: Ty<'tcx>,
variant: &'tcx ty::VariantDef,
substs: &'tcx Substs<'tcx>,
is_simd: bool,
span: Span,
}

impl<'tcx> StructMemberDescriptionFactory<'tcx> {
fn create_member_descriptions<'a>(&self, cx: &CrateContext<'a, 'tcx>)
-> Vec<MemberDescription> {
let field_size = if self.is_simd {
let fty = monomorphize::field_ty(cx.tcx(),
self.substs,
&self.variant.fields[0]);
Some(machine::llsize_of_alloc(
cx,
type_of::type_of(cx, fty)
) as usize)
} else {
None
let layout = cx.layout_of(self.ty);

let tmp;
let offsets = match *layout {
layout::Univariant { ref variant, .. } => &variant.offsets,
layout::Vector { element, count } => {
let element_size = element.size(&cx.tcx().data_layout).bytes();
tmp = (0..count).
map(|i| layout::Size::from_bytes(i*element_size))
.collect::<Vec<layout::Size>>();
&tmp
}
_ => bug!("{} is not a struct", self.ty)
};

self.variant.fields.iter().enumerate().map(|(i, f)| {
Expand All @@ -910,11 +913,7 @@ impl<'tcx> StructMemberDescriptionFactory<'tcx> {
};
let fty = monomorphize::field_ty(cx.tcx(), self.substs, f);

let offset = if self.is_simd {
FixedMemberOffset { bytes: i * field_size.unwrap() }
} else {
ComputedMemberOffset
};
let offset = FixedMemberOffset { bytes: offsets[i].bytes() as usize};

MemberDescription {
name: name,
Expand Down Expand Up @@ -956,9 +955,9 @@ fn prepare_struct_metadata<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
struct_metadata_stub,
struct_llvm_type,
StructMDF(StructMemberDescriptionFactory {
ty: struct_type,
variant: variant,
substs: substs,
is_simd: struct_type.is_simd(),
span: span,
})
)
Expand All @@ -970,13 +969,21 @@ fn prepare_struct_metadata<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,

// Creates MemberDescriptions for the fields of a tuple
struct TupleMemberDescriptionFactory<'tcx> {
ty: Ty<'tcx>,
component_types: Vec<Ty<'tcx>>,
span: Span,
}

impl<'tcx> TupleMemberDescriptionFactory<'tcx> {
fn create_member_descriptions<'a>(&self, cx: &CrateContext<'a, 'tcx>)
-> Vec<MemberDescription> {
let layout = cx.layout_of(self.ty);
let offsets = if let layout::Univariant { ref variant, .. } = *layout {
&variant.offsets
} else {
bug!("{} is not a tuple", self.ty);
};

self.component_types
.iter()
.enumerate()
Expand All @@ -985,7 +992,7 @@ impl<'tcx> TupleMemberDescriptionFactory<'tcx> {
name: format!("__{}", i),
llvm_type: type_of::type_of(cx, component_type),
type_metadata: type_metadata(cx, component_type, self.span),
offset: ComputedMemberOffset,
offset: FixedMemberOffset { bytes: offsets[i].bytes() as usize },
flags: DIFlags::FlagZero,
}
}).collect()
Expand All @@ -1012,6 +1019,7 @@ fn prepare_tuple_metadata<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
NO_SCOPE_METADATA),
tuple_llvm_type,
TupleMDF(TupleMemberDescriptionFactory {
ty: tuple_type,
component_types: component_types.to_vec(),
span: span,
})
Expand Down Expand Up @@ -1250,7 +1258,7 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> {
},
layout::StructWrappedNullablePointer { nonnull: ref struct_def,
nndiscr,
ref discrfield, ..} => {
ref discrfield_source, ..} => {
// Create a description of the non-null variant
let (variant_type_metadata, variant_llvm_type, member_description_factory) =
describe_enum_variant(cx,
Expand All @@ -1273,12 +1281,12 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> {
// member's name.
let null_variant_index = (1 - nndiscr) as usize;
let null_variant_name = adt.variants[null_variant_index].name;
let discrfield = discrfield.iter()
let discrfield_source = discrfield_source.iter()
.skip(1)
.map(|x| x.to_string())
.collect::<Vec<_>>().join("$");
let union_member_name = format!("RUST$ENCODED$ENUM${}${}",
discrfield,
discrfield_source,
null_variant_name);
Copy link
Member

Choose a reason for hiding this comment

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

Q_Q why doesn't this use an offset? cc @michaelwoerister

Copy link
Member

Choose a reason for hiding this comment

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

I guess it'd need both a type and an offset which it can get from the field path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaving this one for now. This is the old behavior, and we have tests against it that fail if it changes.

If I had to bet, it's field indices because the pretty printers access them with "humanlike" expressions or something.


// Create the (singleton) list of descriptions of union members.
Expand All @@ -1300,6 +1308,8 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> {

// Creates MemberDescriptions for the fields of a single enum variant.
struct VariantMemberDescriptionFactory<'tcx> {
// Cloned from the layout::Struct describing the variant.
offsets: Vec<layout::Size>,
Copy link
Member

Choose a reason for hiding this comment

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

The layouts are interned so you can just use &'tcx [layout::Size].

Copy link
Member

Choose a reason for hiding this comment

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

Still need to address this.

args: Vec<(String, Ty<'tcx>)>,
discriminant_type_metadata: Option<DIType>,
span: Span,
Expand All @@ -1316,7 +1326,7 @@ impl<'tcx> VariantMemberDescriptionFactory<'tcx> {
Some(metadata) if i == 0 => metadata,
_ => type_metadata(cx, ty, self.span)
},
offset: ComputedMemberOffset,
offset: FixedMemberOffset { bytes: self.offsets[i].bytes() as usize },
flags: DIFlags::FlagZero
}
}).collect()
Expand Down Expand Up @@ -1420,6 +1430,7 @@ fn describe_enum_variant<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,

let member_description_factory =
VariantMDF(VariantMemberDescriptionFactory {
offsets: struct_def.offsets.clone(),
args: args,
discriminant_type_metadata: match discriminant_info {
RegularDiscriminant(discriminant_type_metadata) => {
Expand Down
10 changes: 8 additions & 2 deletions src/librustc_trans/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use llvm::{self, ValueRef};
use rustc_const_eval::{ErrKind, ConstEvalErr, note_const_eval_err};
use rustc::middle::lang_items;
use rustc::ty;
use rustc::ty::{self, layout};
use rustc::mir;
use abi::{Abi, FnType, ArgType};
use adt;
Expand Down Expand Up @@ -722,8 +722,14 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {

}
Immediate(llval) => {
let l = bcx.ccx().layout_of(tuple.ty);
let v = if let layout::Univariant { ref variant, .. } = *l {
variant
} else {
bug!("Not a tuple.");
};
for (n, &ty) in arg_types.iter().enumerate() {
let mut elem = bcx.extract_value(llval, n);
let mut elem = bcx.extract_value(llval, v.memory_index[n] as usize);
// Truncate bools to i1, if needed
if ty.is_bool() && common::val_ty(elem) != Type::i1(bcx.ccx()) {
elem = bcx.trunc(elem, Type::i1(bcx.ccx()));
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/mir/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
lldest: ValueRef,
operand: OperandRef<'tcx>)
{
debug!("store_operand: operand={:?}", operand);
debug!("store_operand: operand={:?} lldest={:?}", operand, lldest);
bcx.with_block(|bcx| self.store_operand_direct(bcx, lldest, operand))
}

Expand Down
12 changes: 12 additions & 0 deletions src/librustc_trans/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,25 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
}
},
_ => {
// If this is a tuple or closure, we need to translate GEP indices.
let layout = bcx.ccx().layout_of(dest.ty.to_ty(bcx.tcx()));
let translation = if let Layout::Univariant { ref variant, .. } = *layout {
Some(&variant.memory_index)
} else {
None
};
for (i, operand) in operands.iter().enumerate() {
let op = self.trans_operand(&bcx, operand);
// Do not generate stores and GEPis for zero-sized fields.
if !common::type_is_zero_size(bcx.ccx(), op.ty) {
// Note: perhaps this should be StructGep, but
// note that in some cases the values here will
// not be structs but arrays.
let i = if let Some(ref t) = translation {
t[i] as usize
} else {
i
};
let dest = bcx.gepi(dest.llval, &[0, i]);
self.store_operand(&bcx, dest, op);
}
Expand Down
Loading