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

[Peephole Optimization 1/n] Don't allocate for structs with a single primval field #146

Merged
merged 2 commits into from
Mar 14, 2017

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 24, 2017

No description provided.

// enums with just one variant are no different, but `.struct_variant()` doesn't work for enums
let variant = &def.variants[0];
// FIXME: also allow structs with only a single non zst field
if variant.fields.len() == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

let fields = variant.fields.iter().filter(|field|self.type_size(.ty(self.tcx, substs)) != Ok(Some(0))).collect::<Vec<_>>();
if fields.len() == 1{
//...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it's not that easy, other code also has the invariant that we treat zst fields as real fields.

@@ -490,7 +490,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
args.push((undef, field_ty));
}
},
_ => bug!("rust-call ABI tuple argument was {:?}", last),
_ => bug!("rust-call ABI tuple argument was {:?}, but {:?} were expected", last, fields),
Copy link
Member

Choose a reason for hiding this comment

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

s/were/was?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well... the fields are multiple xD not sure what the exact grammar is.

@eddyb
Copy link
Member

eddyb commented Feb 24, 2017

FWIW I'd love to have this encoded in ty::Layout, and there's another simplification we can do:
Once rust-lang/rust#39999 gets merged, we'll more or less be forced to handle padding at arbitrary locations, and we can extend this to also allow padding before the first field.
That would allow making enum variants not contain an implicit discriminant field, but instead be accessed like any struct - and MIR already works like this, SetDiscriminant is on the enum while variant fields use Downcast projections.
On top of all that, we can add "variant types", i.e. TyAdt with a variant index, which could have their own Layout::Univariant with the front padding where the discriminant would go.

How does this relate to "newtype unpacking"? Well, we already have something like that!
Layout::StructWrappedNullablePointer is the general "zero value enum discriminant optimization" form, whereas Layout::RawNullablePointer is a (one-level, sadly) newtype special-case.
If we had variant types, there'd be a single Layout variant for the "niche (zero only atm) value optimization", with the variant type distinguishing between Univariant and Newtype, as with a struct.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 24, 2017

allow padding before the first field.

Offsets before the first field are already representable by layout::Struct::offsets. (And miri would treat them correctly with the existing code to best of my knowledge)

FWIW I'd love to have this encoded in ty::Layout,

Do you mean another variant of that enum? or encoded in the existing variants?

On top of all that, we can add "variant types", i.e. TyAdt with a variant index, which could have their own Layout::Univariant with the front padding where the discriminant would go.

Isn't that the closed RFC about enum variants being their own types?

@eddyb
Copy link
Member

eddyb commented Feb 24, 2017

Offsets before the first field are already representable by layout::Struct::offsets.

Correct, but we don't have the logic to generate the right LLVM types (rust-lang/rust#39999 gets close).

Do you mean another variant of that enum? or encoded in the existing variants?

New variant, as an alternative to Univariant.

Isn't that the closed RFC about enum variants being their own types?

Indeed! However, we can use it inside the compiler to replace LvalueTy and not expose it to the user.

@solson
Copy link
Member

solson commented Mar 13, 2017

This is an optimization I wanted, and I'm fine with merging this as-is, but I definitely want to find some way to make it more general and avoid special-case checks for 1 field (or 1 non-ZST field) all over the place. Hopefully it could also combine with some of the 2-field ByValPair checks.

I haven't digested everything @eddyb said, but it'd be nice if rustc could help us out here.

@solson
Copy link
Member

solson commented Mar 13, 2017

@eddyb @oli-obk Do you think we should merge right now, or do you want to wait and try a different approach based on what @eddyb is saying?

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 14, 2017

I've tried implementing this in rustc, but I'm not sure if my design is sensible, since apparently it's easy to miss some cases and then stage2 segfaults. I added a new Layout variant that just contains a FieldPath pointing to the relevant field. Whenever one hits that variant, one needs to unwrap it until the real field is reached... Didn't come up with anything better

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 14, 2017

This is an optimization I wanted, and I'm fine with merging this as-is, but I definitely want to find some way to make it more general and avoid special-case checks for 1 field (or 1 non-ZST field) all over the place. Hopefully it could also combine with some of the 2-field ByValPair checks.

I have improvements coming on top of this PR for 2 field optimizations and cleanups simplifying everything. But I want to do this incrementally.

@solson solson merged commit 0ca0676 into rust-lang:master Mar 14, 2017
@oli-obk oli-obk deleted the peephole1 branch March 17, 2017 09:35
erickt pushed a commit to erickt/miri that referenced this pull request Feb 4, 2022
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.

4 participants