-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 type memory layouts and ABIs, to be more general and easier to optimize. #45225
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
Started a crater run (we'll run cargobomb later, but that takes longer). |
holy shit |
} else if let layout::Abi::ScalarPair(ref a, ref b) = self.layout.abi { | ||
// Offsets have to match either first or second field. | ||
assert_eq!(offset, a.value.size(ccx).abi_align(b.value.align(ccx))); | ||
bcx.struct_gep(self.llval, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this always 1? Because you do a pointercast instead of a gep in the offset 0 case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and the assert ensures it.
@@ -1,14 +0,0 @@ | |||
// Copyright 2013 The Rust Project Developers. See the COPYRIGHT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of those things that don't trigger a typeck error but is totally broken, and I didn't want to deal with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "totally broken" mean in this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something hid the ICE and #30276 was closed, but that was accidental. This is more or less a bug in WF-checking of function arguments in signatures, which @nikomatsakis somewhat insisted on keeping around, which allows fn(Unsized)
to even exist (it shouldn't, for now).
@@ -54,9 +54,6 @@ pub struct PackedPair(u8, u32); | |||
// CHECK-LABEL: @pkd_pair | |||
#[no_mangle] | |||
pub fn pkd_pair(pair1: &mut PackedPair, pair2: &mut PackedPair) { | |||
// CHECK: [[V1:%[a-z0-9]+]] = load i8, i8* %{{.*}}, align 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the case this test was intended to check unreachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, yeah, ScalarPair
only works by having the placement of the second scalar at an ABI-aligned offset, so it's incompatible with packing.
src/librustc/ty/layout.rs
Outdated
} | ||
/// Describes how the fields of a type are located in memory. | ||
#[derive(PartialEq, Eq, Hash, Debug)] | ||
pub enum FieldPlacement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do "general" enums use FieldPlacement::Union
for the discriminant only and "niche" enums use FieldPlacement::Arbitrary
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because a tag is always at offset
0
whereas a niche can be at some offset - we could have a variant of FieldPlacement
that's just one Size
to avoid the Vec
but that's about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why we can't use FieldPlacement::Arbitrary
for both of them? "Normal" structs with one field don't suddenly use FieldPlacement::Union
because they can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, FieldPlacement::Arbitrary
is a bit of a waste, and I considered using FieldPlacement::Union
for normal structs too, but then I have checks for Union
in some places which would conflict with that now. I think you're right and I should make the change here.
EDIT: done.
EDIT2: That's funny, not using FieldPlacement::Union
results in some missed optimizations (codegen/{alloc-optimisation,vec-optimizes-away}
). I'll try to see what I can do about it.
EDIT3: None of the tricks I've tried work, I'll just leave it like this to avoid pessimizing tagged enums for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment to that effect?
c0e7215
to
08eb7c7
Compare
I've just moved |
a5b5722
to
d595119
Compare
☔ The latest upstream changes (presumably #45031) made this pull request unmergeable. Please resolve the merge conflicts. |
@mrhota Are you sure you meant #6791? That issue was closed more than three years ago. The problem with #31215 and #36237 was that they only added code where this PR is a net negative. This PR implements most of rust-lang/rfcs#1230, the description there is confused in places (i.e. it's a list of special cases, but several of them happen to be the exact same optimization, just scattered).
EDIT: for an updated view on these hard cases, see rust-lang/rfcs#1230 (comment). My approach with this PR has been to avoid doing anything that would pessimize codegen, and one example of this is that |
I can't reproduce the Travis failure locally, even after commenting out most of my |
@alexcrichton @Mark-Simulacrum I hope you don't mind that I just pushed a temporary commit to build on Travis without LLVM 3.7, to check if it's some old LLVM bug getting triggered. |
4c4d081
to
5d92b1a
Compare
Confirmed successful build with newer (4.0?) LLVM. I'll try to reproduce locally. |
@alexcrichton @Mark-Simulacrum May I suggest outputting a signal number by default?
But if I turn verbose mode on, there's an extra (manually wrapped for your convenience):
Printing just " |
The DWARF generated for Rust enums was always somewhat unusual. Rather than using DWARF constructs directly, it would emit magic field names like "RUST$ENCODED$ENUM$0$Name" and "RUST$ENUM$DISR". Since PR rust-lang#45225, though, even this has not worked -- the ad hoc scheme was not updated to handle the wider variety of niche-filling layout optimizations now available. This patch changes the generated DWARF to use the standard tags meant for this purpose; namely, DW_TAG_variant and DW_TAG_variant_part. The patch to implement this went in to LLVM 7. In order to work with older versions of LLVM, and because LLVM doesn't do anything here for PDB, the existing code is kept as a fallback mode. Support for this DWARF is in the Rust lldb and in gdb 8.2. Closes rust-lang#32920 Closes rust-lang#32924 Closes rust-lang#52762 Closes rust-lang#53153
Fix DWARF generation for enums The DWARF generated for Rust enums was always somewhat unusual. Rather than using DWARF constructs directly, it would emit magic field names like "RUST$ENCODED$ENUM$0$Name" and "RUST$ENUM$DISR". Since PR #45225, though, even this has not worked -- the ad hoc scheme was not updated to handle the wider variety of niche-filling layout optimizations now available. This patch changes the generated DWARF to use the standard tags meant for this purpose; namely, DW_TAG_variant and DW_TAG_variant_part. The patch to implement this went in to LLVM 7. In order to work with older versions of LLVM, and because LLVM doesn't do anything here for PDB, the existing code is kept as a fallback mode. Support for this DWARF is in the Rust lldb and in gdb 8.2. Closes #32920 Closes #32924 Closes #52762 Closes #53153
The DWARF generated for Rust enums was always somewhat unusual. Rather than using DWARF constructs directly, it would emit magic field names like "RUST$ENCODED$ENUM$0$Name" and "RUST$ENUM$DISR". Since PR rust-lang#45225, though, even this has not worked -- the ad hoc scheme was not updated to handle the wider variety of niche-filling layout optimizations now available. This patch changes the generated DWARF to use the standard tags meant for this purpose; namely, DW_TAG_variant and DW_TAG_variant_part. The patch to implement this went in to LLVM 7. In order to work with older versions of LLVM, and because LLVM doesn't do anything here for PDB, the existing code is kept as a fallback mode. Support for this DWARF is in the Rust lldb and in gdb 8.2. Closes rust-lang#32920 Closes rust-lang#32924 Closes rust-lang#52762 Closes rust-lang#53153
Fix DWARF generation for enums The DWARF generated for Rust enums was always somewhat unusual. Rather than using DWARF constructs directly, it would emit magic field names like "RUST$ENCODED$ENUM$0$Name" and "RUST$ENUM$DISR". Since PR #45225, though, even this has not worked -- the ad hoc scheme was not updated to handle the wider variety of niche-filling layout optimizations now available. This patch changes the generated DWARF to use the standard tags meant for this purpose; namely, DW_TAG_variant and DW_TAG_variant_part. The patch to implement this went in to LLVM 7. In order to work with older versions of LLVM, and because LLVM doesn't do anything here for PDB, the existing code is kept as a fallback mode. Support for this DWARF is in the Rust lldb and in gdb 8.2. Closes #32920 Closes #32924 Closes #52762 Closes #53153
The DWARF generated for Rust enums was always somewhat unusual. Rather than using DWARF constructs directly, it would emit magic field names like "RUST$ENCODED$ENUM$0$Name" and "RUST$ENUM$DISR". Since PR rust-lang#45225, though, even this has not worked -- the ad hoc scheme was not updated to handle the wider variety of niche-filling layout optimizations now available. This patch changes the generated DWARF to use the standard tags meant for this purpose; namely, DW_TAG_variant and DW_TAG_variant_part. The patch to implement this went in to LLVM 7. In order to work with older versions of LLVM, and because LLVM doesn't do anything here for PDB, the existing code is kept as a fallback mode. Support for this DWARF is in the Rust lldb and in gdb 8.2. Closes rust-lang#32920 Closes rust-lang#32924 Closes rust-lang#52762 Closes rust-lang#53153
Fix DWARF generation for enums The DWARF generated for Rust enums was always somewhat unusual. Rather than using DWARF constructs directly, it would emit magic field names like "RUST$ENCODED$ENUM$0$Name" and "RUST$ENUM$DISR". Since PR #45225, though, even this has not worked -- the ad hoc scheme was not updated to handle the wider variety of niche-filling layout optimizations now available. This patch changes the generated DWARF to use the standard tags meant for this purpose; namely, DW_TAG_variant and DW_TAG_variant_part. The patch to implement this went in to LLVM 7. In order to work with older versions of LLVM, and because LLVM doesn't do anything here for PDB, the existing code is kept as a fallback mode. Support for this DWARF is in the Rust lldb and in gdb 8.2. Closes #32920 Closes #32924 Closes #52762 Closes #53153
n Rust, an enum that carries data in the variants is, essentially, a discriminated union. Furthermore, the Rust compiler will perform space optimizations on such enums in some situations. Previously, DWARF for these constructs was emitted using a hack (a magic field name); but this approach stopped working when more space optimizations were added in rust-lang/rust#45225. This patch changes LLVM to allow discriminated unions to be represented in DWARF. It adds createDiscriminatedUnionType and createDiscriminatedMemberType to DIBuilder and then arranges for this to be emitted using DWARF's DW_TAG_variant_part and DW_TAG_variant. Note that DWARF requires that a discriminated union be represented as a structure with a variant part. However, as Rust only needs to emit pure discriminated unions, this is what I chose to expose on DIBuilder. Patch by Tom Tromey! Differential Revision: https://reviews.llvm.org/D42082 llvm-svn: 324426
…tly Rust (from servo:stylo-size-of); r=emilio This is on top of servo/servo#19285. Rust Nightly has new enum memory layout optimizations: rust-lang/rust#45225 Source-Repo: https://github.com/servo/servo Source-Revision: 17e97b9320fdb7cdb33bbc5f4d0fde0653bbf2e4 UltraBlame original commit: 6de571030d5d998dcadbd3dac602fa006395165c
…tly Rust (from servo:stylo-size-of); r=emilio This is on top of servo/servo#19285. Rust Nightly has new enum memory layout optimizations: rust-lang/rust#45225 Source-Repo: https://github.com/servo/servo Source-Revision: 17e97b9320fdb7cdb33bbc5f4d0fde0653bbf2e4 UltraBlame original commit: 6de571030d5d998dcadbd3dac602fa006395165c
…tly Rust (from servo:stylo-size-of); r=emilio This is on top of servo/servo#19285. Rust Nightly has new enum memory layout optimizations: rust-lang/rust#45225 Source-Repo: https://github.com/servo/servo Source-Revision: 17e97b9320fdb7cdb33bbc5f4d0fde0653bbf2e4 UltraBlame original commit: 6de571030d5d998dcadbd3dac602fa006395165c
To combat combinatorial explosion, type layouts are now described through 3 orthogonal properties:
Variants
describes the plurality of sum types (where applicable)Single
is for one inhabited/active variant, including all Cstruct
s andunion
sTagged
has its variants discriminated by an integer tag, including Cenum
sNicheFilling
uses otherwise-invalid values ("niches") for all but one of its inhabited variantsFieldPlacement
describes the number and memory offsets of fields (if any)Union
has all its fields at offset0
Array
has offsets that are a multiple of itsstride
; guarantees all fields have one typeArbitrary
records all the field offsets, which can be out-of-orderAbi
describes how values of the type should be passed around, including for FFIUninhabited
corresponds to no values, associated with unreachable control-flowScalar
is ABI-identical to its only integer/floating-point/pointer "scalar component"ScalarPair
has two "scalar components", but only applies to the Rust ABIVector
is for SIMD vectors, typically#[repr(simd)]
struct
s in RustAggregate
has arbitrary contents, including all non-transparent Cstruct
s andunion
sSize optimizations implemented so far:
Option<!>
is 0 bytesResult<T, !>
has the same size asT
0
, to represent a data-less variant, e.g.:Option<bool>
,Option<Option<bool>>
,Option<Ordering>
are all 1 byteOption<char>
is 4 bytesenum E { A(bool), B, C, D }
is 1 byteCode generation now takes advantage of
Scalar
andScalarPair
to, in more cases, pass around scalar components as immediates instead of indirectly, through pointers into temporary memory, while avoiding LLVM's "first-class aggregates", and there's more untapped potential here.Closes #44426, fixes #5977, fixes #14540, fixes #43278.