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

refactor to remove trans::adt and make rustc::ty::layout authoritative #36151

Merged
merged 5 commits into from Sep 26, 2016

Conversation

@camlorn
Copy link
Contributor

camlorn commented Aug 30, 2016

I asked on IRC about optimizing struct layout by reordering fields from most-aligned to least-aligned and somehow ended up getting talked into doing this. The goal here is to make layout authoritative and to remove adt. The former has been accomplished by reimplementing represent_type_uncached and the latter is in progress. @eddyb thought I should make the PR now.

My plan is to reserve the actual optimization for a second PR, as this work is useful by itself.

@rust-highfive
Copy link
Collaborator

rust-highfive commented Aug 30, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@eddyb
Copy link
Member

eddyb commented Aug 30, 2016

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned arielb1 Aug 30, 2016
from: &layout::Struct,
size: layout::Size,
variant: Option<ty::VariantDef<'tcx>>,
substs: &Substs<'tcx>)->Struct<'tcx> {

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

The arguments should be indented 4 more to the right (to align with tcx: ...) and there should be spaces around ->.

if cases.len() == 1 && hint == attr::ReprAny {
// Equivalent to a struct/tuple/newtype.
return Univariant(mk_struct(cx, &cases[0].tys, false, t));
else {

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

The else should be after the }, i.e. the line should be } else {.

// Equivalent to a struct/tuple/newtype.
return Univariant(mk_struct(cx, &cases[0].tys, false, t));
else {
fields = Vec::default();

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

You can do let fields = if let ... { ... } else { ... }; (but on multiple lines like you have now).

}
Struct{size: size_bytes, sized: sized, align: align, packed: packed, fields: fields}

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

The struct initializer should be on multiple lines, e.g.:

Struct {
    a: a,
    b: b
}

Also, if you change this, you don't need local variables for anything other than fields.

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

This whole function also needs to be indented only 4 from the left (it's 12 now).

//This is used to determine the fields of the unsized variant of an enum.
fn compute_null_fields<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
adt: ty::AdtDef<'tcx>,
substs: &Substs<'tcx>)->Vec<Ty<'tcx>> {

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

Same argument misindentation and lack of spaces around ->.

let cases = get_cases(cx.tcx(), adt, substs);
assert_eq!(cases.len(), 2); //More than two variants is not the null pointer optimization.
//We use unwrap heere because it is a bug if this function is called on something wherein the following fails.
let case = cases.iter().filter(|x| x.is_zerolen(cx)).next().unwrap();

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

Shouldn't use is_zerolen, nndiscr is the index of the non-null variant, so the null variant has index 1 - nndiscr.

}
} else {
min_ity
match(&t.sty, layout) {

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

Missing space between match and (.

min_ity
match(&t.sty, layout) {
(_, &Layout::CEnum{discr, signed, min, max}) => {
return Repr::CEnum(integer_to_int_type(discr, signed), Disr(min), Disr(max));

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

None of the match arms here need return, you can just remove return and the semicolon and it will work.

//Also, note that general-case discriminants are always unsigned.
s.fields.reverse();
s.fields.push(front_field);
s.fields.reverse();

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

s.fields.insert(0, front_field) would work.

let sized = nonnull.sized;
let align = nonnull.align.abi() as u32;
let packed = nonnull.packed;
let nonnull_s = Struct{

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

Missing space between Struct and {.

@@ -451,169 +374,30 @@ fn get_cases<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
let field_tys = vi.fields.iter().map(|field| {
monomorphize::field_ty(tcx, substs, field)
}).collect();
Case { discr: Disr::from(vi.disr_val), tys: field_tys }
Case {tys: field_tys }

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

Missing space after {.

@@ -900,8 +684,7 @@ pub fn trans_set_discr<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, r: &Repr<'tcx>,
fn assert_discr_in_range(ity: IntType, min: Disr, max: Disr, discr: Disr) {
match ity {
attr::UnsignedInt(_) => {
assert!(min <= discr);
assert!(discr <= max);
if min <= max { assert!(min <= discr && discr <= max) } else { assert!(min <= discr || discr <= max) }

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

This function shouldn't take an IntType and the if/else should be on multiple lines and properly indented.

bug!("failed to get layout for `{}`: {}", ty, e);
})
})
}

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

If you rebase on master and force push this should be gone.

@@ -322,4 +344,4 @@ impl TypeNames {
pub fn find_type(&self, s: &str) -> Option<Type> {
self.named_types.borrow().get(s).map(|x| Type::from_ref(*x))
}
}
}

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

Accidental removal of trailing newline at the end of the file.

@@ -18,7 +18,7 @@ pub fn main() {
let args: Vec<String> = env::args().collect();
if args.len() >= 2 && args[1] == "signal" {
// Raise a segfault.
unsafe { *(0 as *mut isize) = 0; }
unsafe { *(1 as *mut isize) = 0; }

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

Note to anyone who might be looking at this: #36129 has this change, but it's not merged yet.

@camlorn camlorn force-pushed the camlorn:struct_layout_optimization branch 2 times, most recently from 8036624 to f76321e Aug 31, 2016
@camlorn
Copy link
Contributor Author

camlorn commented Aug 31, 2016

I've incorporated the stylistic fixes from @eddyb. For some reason, make tidy is insisting that all source files are binary, so we'll see if the buildbot does that again. I need to find out what it's looking for. This did pick up a few lines that were too long. I'll try to be more careful about indentation in future.

The Case struct has now died.

I'm going to continue by trying to get debuginfo to use layout instead of adt, but may need help. I tripped on it yesterday, but I think that I started trying to refactor it from the wrong place. This then caused one small change to it to actually be rather large in scope. I think that starting from somewhere different might allow me to do it without having to keep the whole module in my head.

I can't continue this until either late this evening or tomorrow.

@@ -939,10 +939,8 @@ impl<'a, 'gcx, 'tcx> Layout {
}
}

if def.variants.len() == 1 {
if def.variants.len() == 1 && hint == attr::ReprAny{

This comment has been minimized.

Copy link
@arielb1

arielb1 Aug 31, 2016

Contributor

nit: missing space after attr::ReprAny

}
}
None
fn integer_to_int_type(from: layout::Integer, signed: bool)->IntType {

This comment has been minimized.

Copy link
@arielb1

arielb1 Aug 31, 2016

Contributor

Why isn't this in ty::layout?

This comment has been minimized.

Copy link
@camlorn

camlorn Aug 31, 2016

Author Contributor

Because my goal is to be rid of it entirely. It was a helper function introduced to deal with Repr's choice to use attr::IntType. Once the Repr type is removed, that function should be going away. Generally, I think going backwards like that is bad (but do tell me if you know of a good reason).

This comment has been minimized.

Copy link
@arielb1

arielb1 Aug 31, 2016

Contributor

So you are going to be all-unsigned? I guess that LLVM does not have signed integers.

This comment has been minimized.

Copy link
@camlorn

camlorn Aug 31, 2016

Author Contributor

Yeah. The code already erases it at this level. There might be some places that don't, but I haven't seen them yet and it's probably only something in debuginfo.

}
}

fn mk_mach_int_from_layout<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

This comment has been minimized.

Copy link
@arielb1

arielb1 Aug 31, 2016

Contributor

this too.

This comment has been minimized.

Copy link
@camlorn

camlorn Aug 31, 2016

Author Contributor

I agree on this one; seems kind of silly in hindsight.

use syntax::attr;
use syntax::attr::IntType;
use abi::FAT_PTR_ADDR;
use build::*;
use common::*;
use debuginfo::DebugLoc;

This comment has been minimized.

Copy link
@arielb1

arielb1 Aug 31, 2016

Contributor

Why is Repr still a thing? It does not seem to contain any information (except that Repr is equiv to (Layout, Ty)).

This comment has been minimized.

Copy link
@camlorn

camlorn Aug 31, 2016

Author Contributor

I haven't completed this PR yet. @eddyb thought I should make it anyway so we would get diff comments and the like. Making Repr not a thing is the goal, but I haven't finished and lots of places still insist on using it.

@@ -1569,7 +1568,9 @@ fn prepare_enum_metadata<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
match cached_discriminant_type_metadata {
Some(discriminant_type_metadata) => discriminant_type_metadata,
None => {
let discriminant_llvm_type = adt::ll_inttype(cx, inttype);
let discriminant_llvm_type = Type::from_integer(cx,

This comment has been minimized.

Copy link
@arielb1

arielb1 Aug 31, 2016

Contributor

Why not move ll_inttype to Type?

This comment has been minimized.

Copy link
@camlorn

camlorn Aug 31, 2016

Author Contributor

It will be when I'm done, but it also needs to be recoded to deal with layout::Integer.

@@ -299,6 +300,27 @@ impl Type {
llvm::LLVMGetIntTypeWidth(self.to_ref()) as u64
}
}

pub fn from_integer(cx: &CrateContext, i: layout::Integer)->Type {

This comment has been minimized.

Copy link
@arielb1

arielb1 Aug 31, 2016

Contributor

Doesn't this need a signed bit?

This comment has been minimized.

Copy link
@camlorn

camlorn Aug 31, 2016

Author Contributor

I don't think so. @eddyb says that at the LLVM level, the distinction isn't made. You can confirm this by observing that int_from_ty and uint_from_ty do the same thing.

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Needs spaces around ->.

nndiscr: u64) -> Vec<Ty<'tcx>> {
let cases = get_cases(cx.tcx(), adt, substs);
assert_eq!(cases.len(), 2); //More than two variants is not the null pointer optimization.
cases[1-(nndiscr as usize)].clone()

This comment has been minimized.

Copy link
@eddyb

eddyb Aug 31, 2016

Member

Eventually get_cases should be avoided, IMO, and this would look at the variant directly.

@camlorn
Copy link
Contributor Author

camlorn commented Sep 13, 2016

@arielb1
I'll look at the overflow stuff. @eddyb told me he thought layout was already handling the case of types that are too big to fit in the address space.

@camlorn
Copy link
Contributor Author

camlorn commented Sep 13, 2016

@arielb1
Nevermind, I'm being an idiot. I see what you mean in regards to not catching it. I suppose that the solution here is to spit out a compiler error. Where do I need to look to understand how to go about doing this?

@eddyb
Copy link
Member

eddyb commented Sep 13, 2016

@camlorn Look for how we check for size overflows when computing the LLVM types.
CrateContext::layout_of needs to do that error reporting, AFAICT.

@arielb1
Copy link
Contributor

arielb1 commented Sep 14, 2016

Needs rebase

@@ -496,6 +496,9 @@ pub struct GlobalCtxt<'tcx> {
/// Cache for layouts computed from types.
pub layout_cache: RefCell<FnvHashMap<Ty<'tcx>, &'tcx Layout>>,

//Used to prevent layout from recursing too deeply.

This comment has been minimized.

Copy link
@arielb1

arielb1 Sep 14, 2016

Contributor

nit: don't put random mutable parameters on the tcx. Use a LayoutContext.

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 14, 2016

Member

You can't, because the recursion doesn't go through a dedicated context. You might be able to put it in TargetDataLayout, I guess, but that's about it.

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Space after //. Also, /// should be used (signifying doc comments).

@camlorn camlorn force-pushed the camlorn:struct_layout_optimization branch from 1094330 to 0561f6f Sep 16, 2016
layout::Size::from_bytes(i*element_size.bytes())
}).collect::<Vec<_>>();
let contents = build_const_struct(ccx, &offsets[..], vals, true);
C_struct(ccx, &contents[..], true)

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 18, 2016

Member

It shouldn't be C_struct but rather a LLVM vector type.

This comment has been minimized.

Copy link
@camlorn

camlorn Sep 19, 2016

Author Contributor

It was C_struct before, as layout::Vector went to Repr::Univariant. I'll see if I can figure out how to change this. But I half-suspect it'll cause all the simd tests to start failing (presumably LLVM treats vector types differently and presumably other stuff still assumes it's whatever C_struct does).

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 19, 2016

Member

It shouldn't have gone through Repr, I don't think. Ah, but it's fine because of the way we handle constants, that is, we cast a pointer to a location holding the constant, because enum variants aren't the type of the enum (and in this case, structs instead of vectors).

This comment has been minimized.

Copy link
@camlorn

camlorn Sep 19, 2016

Author Contributor

Are you saying leave it or change it? I follow your explanation, but I'm not sure if you still consider it a problem worthy of being fixed.

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 19, 2016

Member

It's fine to leave it as it was before, however, it may actually cleaner to use a (LLVM) vector and not generate the Vec of field types.

1 => Type::array(&Type::i8(cx), align_units),
2 => Type::array(&Type::i16(cx), align_units),
4 => Type::array(&Type::i32(cx), align_units),
8 if machine::llalign_of_min(cx, Type::i64(cx)) == 8 =>

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 20, 2016

Member

This can use cx.tcx().data_layout.i64_align.abi() - in fact, look in ty::layout for how a similar match (for enums) was turned into a more general thing (e.g. there are architectures where i32 doesn't have an alignment of 4).

@bors
Copy link
Contributor

bors commented Sep 22, 2016

The latest upstream changes (presumably #36496) made this pull request unmergeable. Please resolve the merge conflicts.

@camlorn camlorn force-pushed the camlorn:struct_layout_optimization branch from 1d6e735 to cfadf74 Sep 23, 2016
@camlorn
Copy link
Contributor Author

camlorn commented Sep 23, 2016

Repr is dead.

Don't merge this yet. generic_type_of still needs cleanup. I may also go after Disr, but haven't looked to see how bad that is as of yet.

adt probably needs splitting, reorganization, and maybe outright elimination. I am open to suggestions as to what module(s) to create from it, but am perfectly fine with leaving it as it is (this PR now technically meets the goal of making layout authoritative and enabling us to more easily add optimizations).

Throw your comments in for additional cleanup, and I'll incorporate them. At this point, I'm not sure what all I should try to kill still, or if there's anything else I'm not getting an unused warning about that's not being called.

}
}

fn struct_field_ptr<'blk, 'tcx>(bcx: &BlockAndBuilder<'blk, 'tcx>,
st: &Struct<'tcx>, val: MaybeSizedValue,
st: &layout::Struct, fields: &Vec<Ty<'tcx>>, val: MaybeSizedValue,

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 23, 2016

Member

Creating vectors is expensive. This function should use the layout::Struct to get the offset of the last field instead.

Copy link
Member

eddyb left a comment

Great job! Mostly only formatting nitpicks left.

@@ -328,6 +328,43 @@ pub enum Integer {
}

impl Integer {

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Extra newline.

}

pub fn to_ty<'a, 'tcx>(&self, tcx: &ty::TyCtxt<'a, 'tcx, 'tcx>,
signed: bool) -> Ty<'tcx> {

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

signed is aligned 4 spaces to the left of where it should be.

@@ -350,6 +387,18 @@ impl Integer {
}
}

//Find the smallest integer with the given alignment.

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Space after // and use ///.

@@ -1003,6 +1061,16 @@ impl<'a, 'gcx, 'tcx> Layout {
}
}

if def.variants.len() == 1 && hint == attr::ReprAny{

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Missing space before {. Also, this will never be reached, maybe you meant to put the hint == attr::ReprAny in the previous if?

//Given an enum, struct, closure, or tuple, extracts fields.
//treats closures as a struct with one variant.
//`empty_if_no_variants` is a switch to deal with empty enums.
//if true, `variant_index` is disregarded and an empty Vec returned in this case.

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Same: space after // and make it /// for doc comments.

General(ity, _) => {
C_integral(ll_inttype(bcx.ccx(), ity), discr.0, true)
layout::RawNullablePointer { .. } |
layout::StructWrappedNullablePointer { .. } => {

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Missing alignment on this line.

for (&val, target_offset) in
vals.iter().zip(
offset_after_field.iter().map(|i| i.bytes())
) {

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

You can put the second iterator in a target_offsets variable to recover the original look.

@@ -1292,11 +1291,15 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> {
fn create_member_descriptions<'a>(&self, cx: &CrateContext<'a, 'tcx>)
-> Vec<MemberDescription> {
let adt = &self.enum_type.ty_adt_def().unwrap();
let substs = match self.enum_type.sty {
ty::TyAdt(def, ref s) if def.adt_kind() == AdtKind::Enum => s,
ref t @ _ => bug!("{} is not an enum", t)

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Print self.enum_type instead of the sty.

@@ -735,7 +733,12 @@ impl<'a, 'tcx> MirConstContext<'a, 'tcx> {

let base = match tr_lvalue.base {
Base::Value(llval) => {
let align = type_of::align_of(self.ccx, ty);
//Fixme: may be wrong for &*(&simd_vec as &fmt::Debug)

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Space after // and FIXME.

}
}

pub fn from_primitive(ccx: &CrateContext, p: layout::Primitive)->Type {

This comment has been minimized.

Copy link
@eddyb

eddyb Sep 25, 2016

Member

Same -> thing here.

@camlorn camlorn force-pushed the camlorn:struct_layout_optimization branch from aa83877 to 467454b Sep 26, 2016
@eddyb
eddyb approved these changes Sep 26, 2016
@eddyb
Copy link
Member

eddyb commented Sep 26, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Sep 26, 2016

📌 Commit 467454b has been approved by eddyb

@bors
Copy link
Contributor

bors commented Sep 26, 2016

Testing commit 467454b with merge 9966397...

bors added a commit that referenced this pull request Sep 26, 2016
refactor to remove trans::adt and make rustc::ty::layout authoritative

I asked on IRC about optimizing struct layout by reordering fields from most-aligned to least-aligned and somehow ended up getting talked into doing this.  The goal here is to make `layout` authoritative and to remove `adt`.  The former has been accomplished by reimplementing `represent_type_uncached` and the latter is in progress.  @eddyb thought I should make the PR now.

My plan is to reserve the actual optimization for a second PR, as this work is useful by itself.
@bors bors merged commit 467454b into rust-lang:master Sep 26, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@camlorn camlorn deleted the camlorn:struct_layout_optimization branch Sep 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.