Forward the ABI of the non-zero sized fields of an union if they have the same ABI #55834

ogoffart · 2018-11-09T22:55:59Z

This is supposed to fix the performence regression of using MaybeUninit in
#54668

rust-highfive · 2018-11-09T22:56:02Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (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.

varkor · 2018-11-09T23:09:48Z

r? @eddyb

eddyb · 2018-11-10T04:46:20Z

src/librustc/ty/layout.rs

+                            if abi == Abi::Uninhabited || size == Size::ZERO {
+                                abi = field.abi.clone();
+                            } else if field.size != Size::ZERO && abi != field.abi  {
+                                abi = Abi::Aggregate{ sized: true };


Missing space before {.

eddyb · 2018-11-10T04:47:03Z

src/librustc/ty/layout.rs

                        size = cmp::max(size, field.size);
                    }

+                    // Use scalar_unit to reset the valid range to the maximal one
+                    // since an union might not be initialised


Need to do this before comparing field.abi above with abi, so we never compare ranges.

eddyb · 2018-11-10T04:47:28Z

cc @rkruppe @nagisa @RalfJung

RalfJung · 2018-11-10T07:53:30Z

src/librustc/ty/layout.rs

@@ -697,6 +697,7 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> {
                    }

                    let mut size = Size::ZERO;
+                    let mut abi = Abi::Uninhabited;


What is the meaining of this initial value?

Notice that a union should NOT be uninhabited even if all of its fields are! That would also be a form of "propagating ranges".

Ah, you are using it as a sentinel value for "no non-ZST field found yet". This warrants a comment. However, we should also never use the Uninhabited layout on unions. So maybe better use an Option here, initialized to None?

I think I prefer Option too.

RalfJung · 2018-11-10T07:54:46Z

src/librustc/ty/layout.rs

+                                _ => field.abi.clone(),
+                            };
+
+                            if abi == Abi::Uninhabited || size == Size::ZERO {


Why is this checking for the size of the entire thing? Or whatever else size is?

Ah, size seems to be the size of the largest field so far. That's worth a comment.
I also do not understand why we need two conditions here, shouldn't this simply be "if we have not set an abi yet"? So, shouldn't the Uninhabited test be enough?

Generally, all this new code needs a comment explaining its purpose.

The size check is for PhantomData. I think we can remove the Uninhabited test, since no fields means size is 0 anyway.

I think we can remove the Uninhabited test, since no fields means size is 0 anyway.

Is (u32, UninhabitedEnum) Uninhabited? What's its size?

It is uninhabited at has size 4. Also see #49298.

RalfJung · 2018-11-10T07:55:28Z

src/librustc/ty/layout.rs

+                                        scalar_unit(y.value),
+                                    )
+                                }
+                                _ => field.abi.clone(),


After this normalization, the ABI is sure to permit all bit patterns, right? There are no restrictions about what can be stored e.g. in a vector ABI type?

src/librustc/ty/layout.rs

ogoffart · 2018-11-10T08:39:56Z

@RalfJung thanks for your review. I fixed the uninhabited case, and added some comments.

src/librustc/ty/layout.rs

RalfJung · 2018-11-10T09:57:43Z

Looks good to me now, but I also have hardly any knowledge about layout computation.

eddyb · 2018-11-10T11:23:34Z

LGTM but I'd like at least of @nagisa or @rkruppe to also sign off on it.

RalfJung

Fine for me, though I think using Option would be clearer.

hanna-kruppe · 2018-11-10T13:24:24Z

src/test/codegen/union-abi.rs

+
+pub union UnionI64x4I64{ a: i64x4, b: i64 }
+
+// CHECK: define void @test_UnionI64x4I64(%UnionI64x4I64* {{.*}}, %UnionI64x4I64* {{.*}} %arg0)


Some of these CHECK lines seem to assume aggregates are passed as in the x86_64 SysV ABI, so if I'm not mistaken they'll fail on some other platforms. See the repr(transparent) codegen tests for what it takes to cover all/most of our targets.

Ironically, the Rust default "ABI" (i.e. fn instead of extern fn) would be more consistent, in that we always pass the inner scalars for Scalar / ScalarPair.

Right, I only tested on x86_64, because that's the only arch i have. (and the test is based of the repr-transparent.rs test.) I guess this check and the next one are architecture specific. What should i do? Just remove them? Or move them somewhere else?

I used the rust ABI and removed the return value for these tests.

What the repr(transparent) tests do is separate tests for each variation of how the C ABI encodes aggregates in LLVM signatures. But that's because for repr(transparent) it's important that it matches the C ABI. This PR, by contrast, is mostly about the Rust ABI and mostly about unions that shouldn't be passed as aggregates. So I don't think it needs as comprehensive coverage of extern "C" as the repr(transparent) tests, as long as we have tests somewhere that ensure that repr(C) unions are passed correctly in extern "C" calls. As long as that is true (@eddyb do you know?) I think it doesn't matter very much.

eddyb · 2018-11-10T16:09:22Z

Hang on, are we checking whether the repr is C or not?
I don't think we can do this optimization for repr(C) unions.

ogoffart · 2018-11-10T17:06:05Z

I just disabled the optimisation for repr(C) union. (but i don't know how i can test them: simd are not repr(c), what else would make a difference?)

eddyb · 2018-11-10T17:15:06Z

An union with a large scalar (e.g. u128) should be passed as an u32 in the Rust ABI, but if you add #[repr(C)] to it, it should be passed by indirection.

eddyb · 2018-11-11T09:09:01Z

@RalfJung There's a method about "are optimizations allowed" on AdtDef that we could check.

ogoffart · 2018-11-11T10:43:54Z

I found ReprOptions::inhibit_enum_layout_opt and ReprOptions::inhibit_struct_field_reordering_opt . I could use inhibit_enum_layout_opt, since the second condition does not matter for unions, but that's not really a good name. Or I could add in impl ReprOptions

    pub fn inhibit_union_opt(&self) -> bool {
        self.c()
    }

if we want to have all the inhibit options at the same place. Should I do that?

eddyb · 2018-11-11T11:16:09Z

@ogoffart I think that's good. Kind of surprised there's not a more general method in there.

All struct-related optimizations (including the newtype ABI ones!) appear to use:

let mut optimize = !repr.inhibit_struct_field_reordering_opt();

Maybe the name should be inhibit_field_abi_opt?

bors · 2018-11-13T09:48:15Z

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

ogoffart · 2018-11-13T10:09:57Z

Please resolve the merge conflicts

Should I rebase, or merge?

nagisa · 2018-11-13T10:14:23Z

Rebase.

…

On Tue, Nov 13, 2018, 12:10 Olivier Goffart ***@***.*** wrote: Please resolve the merge conflicts Should I rebase, or merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#55834 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0sLeeA_yV6UFE5Nxo1zbij2V34pdks5uupqYgaJpZM4YXpDz> .

eddyb · 2018-11-15T11:39:23Z

@bors r+

bors · 2018-11-15T11:39:24Z

📌 Commit c040a48 has been approved by eddyb

Forward the ABI of the non-zero sized fields of an union if they have the same ABI This is supposed to fix the performence regression of using MaybeUninit in rust-lang#54668

bors · 2018-11-18T03:30:31Z

⌛ Testing commit c040a48 with merge f5a924d58abe287646f5f02ae22c5dc8153548c2...

bors · 2018-11-18T05:28:25Z

💔 Test failed - status-appveyor

kennytm · 2018-11-18T05:37:16Z

@bors retry #56034

Forward the ABI of the non-zero sized fields of an union if they have the same ABI This is supposed to fix the performence regression of using MaybeUninit in rust-lang#54668

@ghost

Rollup of 25 pull requests Successful merges: - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets) - #55564 (test/linkage-visibility: Ignore on musl targets) - #55827 (A few tweaks to iterations/collecting) - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI) - #55857 (remove unused dependency) - #55862 (in which the E0618 "expected function" diagnostic gets a makeover) - #55867 (do not panic just because cargo failed) - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef) - #55916 (Make miri value visitor usfeful for mutation) - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code) - #55923 (reword #[test] attribute error on fn items) - #55935 (appveyor: Use VS2017 for all our images) - #55949 (ty: return impl Iterator from Predicate::walk_tys) - #55952 (Update to Clang 7 on CI.) - #55953 (#53488 Refactoring UpvarId) - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors) - #55963 (Stress test for MPSC) - #55968 (Clean up some non-mod-rs stuff.) - #55970 (Miri backtrace improvements) - #56007 (CTFE: dynamically make sure we do not call non-const-fn) - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.) - #56012 (avoid shared ref in UnsafeCell::get) - #56016 (Add VecDeque::resize_with) - #56027 (docs: Add missing backtick in object_safety.rs docs) - #56043 (remove "approx env bounds" if we already know from trait) Failed merges: r? @ghost

Rollup of 25 pull requests Successful merges: - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets) - #55564 (test/linkage-visibility: Ignore on musl targets) - #55827 (A few tweaks to iterations/collecting) - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI) - #55857 (remove unused dependency) - #55862 (in which the E0618 "expected function" diagnostic gets a makeover) - #55867 (do not panic just because cargo failed) - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef) - #55916 (Make miri value visitor useful for mutation) - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code) - #55923 (reword #[test] attribute error on fn items) - #55949 (ty: return impl Iterator from Predicate::walk_tys) - #55952 (Update to Clang 7 on CI.) - #55953 (#53488 Refactoring UpvarId) - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors) - #55963 (Stress test for MPSC) - #55968 (Clean up some non-mod-rs stuff.) - #55970 (Miri backtrace improvements) - #56007 (CTFE: dynamically make sure we do not call non-const-fn) - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.) - #56012 (avoid shared ref in UnsafeCell::get) - #56016 (Add VecDeque::resize_with) - #56027 (docs: Add missing backtick in object_safety.rs docs) - #56043 (remove "approx env bounds" if we already know from trait) - #56059 (Increase `Duration` approximate equal threshold to 1us)

rust-highfive assigned varkor Nov 9, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2018

rust-highfive assigned eddyb and unassigned varkor Nov 9, 2018

eddyb reviewed Nov 10, 2018

View reviewed changes

RalfJung reviewed Nov 10, 2018

View reviewed changes

src/librustc/ty/layout.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 10, 2018

View reviewed changes

src/librustc/ty/layout.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 10, 2018

View reviewed changes

src/librustc/ty/layout.rs Outdated Show resolved Hide resolved

eddyb approved these changes Nov 10, 2018

View reviewed changes

RalfJung approved these changes Nov 10, 2018

View reviewed changes

hanna-kruppe reviewed Nov 10, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2018

RalfJung mentioned this pull request Nov 15, 2018

Use MaybeUninit in libcore #54668

Merged

pietroalbini mentioned this pull request Nov 15, 2018

Rollup of 15 pull requests #55990

Closed

kennytm mentioned this pull request Nov 17, 2018

Rollup of 17 pull requests #56026

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 18, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2018

pietroalbini mentioned this pull request Nov 18, 2018

Rollup of 25 pull requests #56051

Merged

bors merged commit c040a48 into rust-lang:master Nov 19, 2018


		pub union UnionI64x4I64{ a: i64x4, b: i64 }

		// CHECK: define void @test_UnionI64x4I64(%UnionI64x4I64* {{.}}, %UnionI64x4I64 {{.*}} %arg0)

Forward the ABI of the non-zero sized fields of an union if they have the same ABI #55834

Forward the ABI of the non-zero sized fields of an union if they have the same ABI #55834

Conversation

ogoffart commented Nov 9, 2018

rust-highfive commented Nov 9, 2018

varkor commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Nov 10, 2018

Choose a reason for hiding this comment

RalfJung Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Nov 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogoffart commented Nov 10, 2018

RalfJung commented Nov 10, 2018

eddyb commented Nov 10, 2018

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Nov 10, 2018

ogoffart commented Nov 10, 2018

eddyb commented Nov 10, 2018

eddyb commented Nov 11, 2018

ogoffart commented Nov 11, 2018

eddyb commented Nov 11, 2018

bors commented Nov 13, 2018

ogoffart commented Nov 13, 2018

nagisa commented Nov 13, 2018 via email

eddyb commented Nov 15, 2018

bors commented Nov 15, 2018

bors commented Nov 18, 2018

bors commented Nov 18, 2018

kennytm commented Nov 18, 2018

RalfJung Nov 10, 2018 •

edited

Loading

RalfJung Nov 10, 2018 •

edited

Loading