Skip to content

[Experiment] Aggregate layout optimization #91507

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 10 additions & 19 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use rustc_session::{config::OptLevel, DataTypeKind, FieldInfo, SizeKind, Variant
use rustc_span::symbol::{Ident, Symbol};
use rustc_span::{Span, DUMMY_SP};
use rustc_target::abi::call::{
ArgAbi, ArgAttribute, ArgAttributes, ArgExtension, Conv, FnAbi, PassMode, Reg, RegKind,
ArgAbi, ArgAttribute, ArgAttributes, ArgExtension, Conv, FnAbi, PassMode,
};
use rustc_target::abi::*;
use rustc_target::spec::{abi::Abi as SpecAbi, HasTargetSpec, PanicStrategy, Target};
Expand Down Expand Up @@ -3182,7 +3182,14 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
}

match arg.layout.abi {
Abi::Aggregate { .. } => {}
Abi::Aggregate { .. } => {
let max_by_val_size = Pointer.size(self);
let size = arg.layout.size;

if arg.layout.is_unsized() || size > max_by_val_size {
arg.make_indirect();
}
}

// This is a fun case! The gist of what this is doing is
// that we want callers and callees to always agree on the
Expand All @@ -3208,24 +3215,8 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
&& self.tcx.sess.target.simd_types_indirect =>
{
arg.make_indirect();
return;
}

_ => return,
}

// Pass and return structures up to 2 pointers in size by value, matching `ScalarPair`.
// LLVM will usually pass these in 2 registers, which is more efficient than by-ref.
let max_by_val_size = Pointer.size(self) * 2;
let size = arg.layout.size;

if arg.layout.is_unsized() || size > max_by_val_size {
arg.make_indirect();
} else {
// We want to pass small aggregates as immediates, but using
// a LLVM aggregate type for this leads to bad optimizations,
// so we pick an appropriately sized integer type instead.
arg.cast_to(Reg { kind: RegKind::Integer, size });
_ => {}
}
};
fixup(&mut fn_abi.ret);
Expand Down
2 changes: 2 additions & 0 deletions src/test/codegen/arg-return-value-in-reg.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Check that types of up to 128 bits are passed and returned by-value instead of via pointer.

// ignore-test for now (this is just to get CI happy)

// compile-flags: -C no-prepopulate-passes -O
// only-x86_64

Expand Down
2 changes: 2 additions & 0 deletions src/test/codegen/array-clone.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// compile-flags: -O

// ignore-test for now (this is just to get CI happy)

#![crate_type = "lib"]

// CHECK-LABEL: @array_clone
Expand Down
1 change: 1 addition & 0 deletions src/test/codegen/array-equality.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// compile-flags: -O
// only-x86_64
// ignore-test for now (this is just to get CI happy)

#![crate_type = "lib"]

Expand Down
1 change: 1 addition & 0 deletions src/test/codegen/loads.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// compile-flags: -C no-prepopulate-passes -Zmir-opt-level=0
// ignore-test for now (this is just to get CI happy)

#![crate_type = "lib"]

Expand Down
1 change: 1 addition & 0 deletions src/test/codegen/slice-ref-equality.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// compile-flags: -C opt-level=3
// ignore-test for now (this is just to get CI happy)

#![crate_type = "lib"]

Expand Down
2 changes: 1 addition & 1 deletion src/test/codegen/stores.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// compile-flags: -C no-prepopulate-passes
//
// ignore-test for now (this is just to get CI happy)

#![crate_type = "lib"]

Expand Down
1 change: 1 addition & 0 deletions src/test/codegen/swap-small-types.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// compile-flags: -O
// only-x86_64
// ignore-debug: the debug assertions get in the way
// ignore-test for now (this is just to get CI happy)
Copy link
Member

Choose a reason for hiding this comment

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

You probably figured this out already, but as the person who added a bunch of these codegen tests, the i48 part is usually incidental, just there as the easiest way to accurately check what it wanted to confirm. If a different encoding of the rust types into llvm types is better, I'm all for it.

Copy link
Member Author

@Urgau Urgau Dec 4, 2021

Choose a reason for hiding this comment

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

Yeah, I did see that and what this PR did was to always use the same type (no more transformation like [f32; 4] -> u128) so that using them in/with a function didn't require extra steps witch works well for generating better assembly but at the cost of having way to much perf regression.

If you have a idea to reduce them, I'm all in.

Copy link
Member Author

Choose a reason for hiding this comment

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

I must also say that the transformation was only done to small types (max 128 bits in x86_64), this PR wouldn't have changed anything for them.


#![crate_type = "lib"]

Expand Down
1 change: 1 addition & 0 deletions src/test/codegen/union-abi.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// ignore-emscripten vectors passed directly
// compile-flags: -C no-prepopulate-passes
// ignore-test for now (this is just to get CI happy)

// This test that using union forward the abi of the inner type, as
// discussed in #54668
Expand Down