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

ahicks92
Copy link
Contributor

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

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> {
Copy link
Member

Choose a reason for hiding this comment

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

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

@ahicks92 ahicks92 force-pushed the struct_layout_optimization branch 2 times, most recently from 8036624 to f76321e Compare August 31, 2016 16:35
@ahicks92
Copy link
Contributor Author

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{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing space after attr::ReprAny

@ahicks92
Copy link
Contributor Author

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

@ahicks92
Copy link
Contributor Author

@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.
Copy link
Contributor

@arielb1 arielb1 Sep 14, 2016

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@eddyb eddyb Sep 25, 2016

Choose a reason for hiding this comment

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

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

layout::Size::from_bytes(i*element_size.bytes())
}).collect::<Vec<_>>();
let contents = build_const_struct(ccx, &offsets[..], vals, true);
C_struct(ccx, &contents[..], true)
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@eddyb eddyb Sep 19, 2016

Choose a reason for hiding this comment

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

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 =>
Copy link
Member

Choose a reason for hiding this comment

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

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.

@ahicks92
Copy link
Contributor Author

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,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Great job! Mostly only formatting nitpicks left.

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

impl Integer {

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline.

}

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

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

Space after // and use ///.

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

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

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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 { .. } => {
Copy link
Member

Choose a reason for hiding this comment

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

Missing alignment on this line.

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

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

Space after // and FIXME.

}
}

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

Choose a reason for hiding this comment

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

Same -> thing here.

@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
@ahicks92 ahicks92 deleted the struct_layout_optimization branch September 26, 2016 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants