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.

Sign up for GitHub

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

Jump to bottom

Represent Result<usize, Box<T>> as ScalarPair(i64, ptr) #121668

Merged
merged 5 commits into from
Mar 13, 2024
Merged

Represent Result<usize, Box<T>> as ScalarPair(i64, ptr) #121668

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4dabbcb
allow using scalarpair with a common prim of ptr/ptr-sized-int
erikdesjardins Feb 27, 2024
f574c65
simplify common prim computation
erikdesjardins Feb 28, 2024
8e40b17
fix test failure due to differing u64 alignment on different targets
erikdesjardins Feb 28, 2024
5ccada6
make check lines for int/ptr common prim test more permissive
erikdesjardins Mar 7, 2024
9f55200
refine common_prim test
erikdesjardins Mar 13, 2024
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
34 changes: 28 additions & 6 deletions compiler/rustc_abi/src/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,15 +813,37 @@ where
break;
}
};
if let Some(pair) = common_prim {
// This is pretty conservative. We could go fancier
// by conflating things like i32 and u32, or even
// realising that (u8, u8) could just cohabit with
// u16 or even u32.
if pair != (prim, offset) {
if let Some((old_prim, common_offset)) = common_prim {
// All variants must be at the same offset
if offset != common_offset {
common_prim = None;
break;
}
// This is pretty conservative. We could go fancier
// by realising that (u8, u8) could just cohabit with
// u16 or even u32.
let new_prim = match (old_prim, prim) {
// Allow all identical primitives.
(x, y) if x == y => x,
// Allow integers of the same size with differing signedness.
// We arbitrarily choose the signedness of the first variant.
(p @ Primitive::Int(x, _), Primitive::Int(y, _)) if x == y => p,
// Allow integers mixed with pointers of the same layout.
// We must represent this using a pointer, to avoid
// roundtripping pointers through ptrtoint/inttoptr.
(p @ Primitive::Pointer(_), i @ Primitive::Int(..))
| (i @ Primitive::Int(..), p @ Primitive::Pointer(_))
if p.size(dl) == i.size(dl) && p.align(dl) == i.align(dl) =>
{
p
}
_ => {
common_prim = None;
break;
}
};
// We may be updating the primitive here, for example from int->ptr.
common_prim = Some((new_prim, common_offset));
} else {
common_prim = Some((prim, offset));
}
Expand Down
43 changes: 43 additions & 0 deletions tests/codegen/common_prim_int_ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
//@ compile-flags: -O

#![crate_type = "lib"]
scottmcm marked this conversation as resolved.
Show resolved Hide resolved

// Tests that codegen works properly when enums like `Result<usize, Box<()>>`
// are represented as `{ u64, ptr }`, i.e., for `Ok(123)`, `123` is stored
// as a pointer.

// CHECK-LABEL: @insert_int
#[no_mangle]
pub fn insert_int(x: usize) -> Result<usize, Box<()>> {
// CHECK: start:
// CHECK-NEXT: inttoptr i{{[0-9]+}} %x to ptr
// CHECK-NEXT: insertvalue
// CHECK-NEXT: ret { i{{[0-9]+}}, ptr }
Ok(x)
}
Comment on lines +10 to +18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am surprised that this just works, inserting inttoptr/ptrtoint as necessary with only a layout change and no changes to codegen. It feels like something in codegen is a bit too permissive about adding casts whenever it needs to...

Copy link
Member

Choose a reason for hiding this comment

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

This is optimized IR, so it's likely that rustc did something much more boring but LLVM optimized it to this.

Add -C no-prepopulate-passes in the compile-flags if you want to look at what cg_llvm is doing directly.

Copy link
Member

Choose a reason for hiding this comment

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

For example, you get an inttoptr like that after LLVM optimizes a store+load: https://rust.godbolt.org/z/qMPWr7Tzz

Copy link
Member

@scottmcm scottmcm Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(Rust would actually prefer this example to be a GEP off null, like #121242, I think. Not that this PR would need to do that.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did I forget that...

Yeah, the IR we generate just spills to an alloca.


// CHECK-LABEL: @insert_box
#[no_mangle]
pub fn insert_box(x: Box<()>) -> Result<usize, Box<()>> {
// CHECK: start:
// CHECK-NEXT: insertvalue { i{{[0-9]+}}, ptr }
// CHECK-NEXT: ret
Err(x)
}

// CHECK-LABEL: @extract_int
#[no_mangle]
pub unsafe fn extract_int(x: Result<usize, Box<()>>) -> usize {
// CHECK: start:
// CHECK-NEXT: ptrtoint
// CHECK-NEXT: ret
x.unwrap_unchecked()
}

// CHECK-LABEL: @extract_box
#[no_mangle]
pub unsafe fn extract_box(x: Result<usize, Box<()>>) -> Box<()> {
// CHECK: start:
// CHECK-NEXT: ret ptr
x.unwrap_err_unchecked()
}
104 changes: 92 additions & 12 deletions tests/codegen/try_question_mark_nop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,41 @@
#![crate_type = "lib"]
#![feature(try_blocks)]

// These are now NOPs in LLVM 15, presumably thanks to nikic's change mentioned in
// <https://github.com/rust-lang/rust/issues/85133#issuecomment-1072168354>.
// Unfortunately, as of 2022-08-17 they're not yet nops for `u64`s nor `Option`.

use std::ops::ControlFlow::{self, Continue, Break};
use std::ptr::NonNull;

// CHECK-LABEL: @option_nop_match_32
#[no_mangle]
pub fn option_nop_match_32(x: Option<u32>) -> Option<u32> {
// CHECK: start:
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: ret { i32, i32 }
match x {
Some(x) => Some(x),
None => None,
}
}

// CHECK-LABEL: @option_nop_traits_32
#[no_mangle]
pub fn option_nop_traits_32(x: Option<u32>) -> Option<u32> {
// CHECK: start:
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: ret { i32, i32 }
try {
x?
}
}

// CHECK-LABEL: @result_nop_match_32
#[no_mangle]
pub fn result_nop_match_32(x: Result<i32, u32>) -> Result<i32, u32> {
// CHECK: start
// CHECK-NEXT: ret i64 %0
// CHECK: start:
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: ret { i32, i32 }
Comment on lines 35 to +41
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although this is no longer just a ret instruction, this is the fewest possible instructions for scalar pair layout, since you need to pack the two separate arguments into a single return value.

For real, non-noop code, the scalar pair layout is arguably slightly better, since passing the components in separate arguments means that consuming code can just use the second argument directly, instead of having to shift it down from the top 32 bits of an i64 reg.

It was kinda janky in the first place for this test to rely on the fact that integers with differing signedness don't get scalar pair layout; Result<u32, u32> has always gotten the same scalar pair layout (since 1.27) that Result<i32, u32> now gets with this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, agreed it's fine to change this test like this. The important part is that it's not branching, not icmping, not selecting, etc.

match x {
Ok(x) => Ok(x),
Err(x) => Err(x),
Expand All @@ -24,8 +48,60 @@ pub fn result_nop_match_32(x: Result<i32, u32>) -> Result<i32, u32> {
// CHECK-LABEL: @result_nop_traits_32
#[no_mangle]
pub fn result_nop_traits_32(x: Result<i32, u32>) -> Result<i32, u32> {
// CHECK: start
// CHECK-NEXT: ret i64 %0
// CHECK: start:
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: ret { i32, i32 }
try {
x?
}
}

// CHECK-LABEL: @result_nop_match_64
#[no_mangle]
pub fn result_nop_match_64(x: Result<i64, u64>) -> Result<i64, u64> {
// CHECK: start:
// CHECK-NEXT: insertvalue { i64, i64 }
// CHECK-NEXT: insertvalue { i64, i64 }
// CHECK-NEXT: ret { i64, i64 }
match x {
Ok(x) => Ok(x),
Err(x) => Err(x),
}
}

// CHECK-LABEL: @result_nop_traits_64
#[no_mangle]
pub fn result_nop_traits_64(x: Result<i64, u64>) -> Result<i64, u64> {
// CHECK: start:
// CHECK-NEXT: insertvalue { i64, i64 }
// CHECK-NEXT: insertvalue { i64, i64 }
// CHECK-NEXT: ret { i64, i64 }
try {
x?
}
}

// CHECK-LABEL: @result_nop_match_ptr
#[no_mangle]
pub fn result_nop_match_ptr(x: Result<usize, Box<()>>) -> Result<usize, Box<()>> {
// CHECK: start:
// CHECK-NEXT: insertvalue { i{{[0-9]+}}, ptr }
// CHECK-NEXT: insertvalue { i{{[0-9]+}}, ptr }
// CHECK-NEXT: ret
match x {
Ok(x) => Ok(x),
Err(x) => Err(x),
}
}

// CHECK-LABEL: @result_nop_traits_ptr
#[no_mangle]
pub fn result_nop_traits_ptr(x: Result<u64, NonNull<()>>) -> Result<u64, NonNull<()>> {
// CHECK: start:
// CHECK-NEXT: insertvalue { i{{[0-9]+}}, ptr }
// CHECK-NEXT: insertvalue { i{{[0-9]+}}, ptr }
// CHECK-NEXT: ret
try {
x?
}
Expand All @@ -34,8 +110,10 @@ pub fn result_nop_traits_32(x: Result<i32, u32>) -> Result<i32, u32> {
// CHECK-LABEL: @control_flow_nop_match_32
#[no_mangle]
pub fn control_flow_nop_match_32(x: ControlFlow<i32, u32>) -> ControlFlow<i32, u32> {
// CHECK: start
// CHECK-NEXT: ret i64 %0
// CHECK: start:
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: ret { i32, i32 }
match x {
Continue(x) => Continue(x),
Break(x) => Break(x),
Expand All @@ -45,8 +123,10 @@ pub fn control_flow_nop_match_32(x: ControlFlow<i32, u32>) -> ControlFlow<i32, u
// CHECK-LABEL: @control_flow_nop_traits_32
#[no_mangle]
pub fn control_flow_nop_traits_32(x: ControlFlow<i32, u32>) -> ControlFlow<i32, u32> {
// CHECK: start
// CHECK-NEXT: ret i64 %0
// CHECK: start:
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: insertvalue { i32, i32 }
// CHECK-NEXT: ret { i32, i32 }
try {
x?
}
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/layout/enum-scalar-pair-int-ptr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//@ normalize-stderr-test "pref: Align\([1-8] bytes\)" -> "pref: $$PREF_ALIGN"
//@ normalize-stderr-test "Int\(I[0-9]+," -> "Int(I?,"
//@ normalize-stderr-test "valid_range: 0..=[0-9]+" -> "valid_range: $$VALID_RANGE"

//! Enum layout tests related to scalar pairs with an int/ptr common primitive.

#![feature(rustc_attrs)]
#![feature(never_type)]
#![crate_type = "lib"]

#[rustc_layout(abi)]
enum ScalarPairPointerWithInt { //~ERROR: abi: ScalarPair
A(usize),
B(Box<()>),
}

// Negative test--ensure that pointers are not commoned with integers
// of a different size. (Assumes that no target has 8 bit pointers, which
// feels pretty safe.)
#[rustc_layout(abi)]
enum NotScalarPairPointerWithSmallerInt { //~ERROR: abi: Aggregate
A(u8),
B(Box<()>),
}
14 changes: 14 additions & 0 deletions tests/ui/layout/enum-scalar-pair-int-ptr.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: abi: ScalarPair(Initialized { value: Int(I?, false), valid_range: $VALID_RANGE }, Initialized { value: Pointer(AddressSpace(0)), valid_range: $VALID_RANGE })
--> $DIR/enum-scalar-pair-int-ptr.rs:12:1
|
LL | enum ScalarPairPointerWithInt {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: abi: Aggregate { sized: true }
--> $DIR/enum-scalar-pair-int-ptr.rs:21:1
|
LL | enum NotScalarPairPointerWithSmallerInt {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

6 changes: 6 additions & 0 deletions tests/ui/layout/enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,9 @@ enum UninhabitedVariantSpace { //~ERROR: size: Size(16 bytes)
A,
B([u8; 15], !), // make sure there is space being reserved for this field.
}

#[rustc_layout(abi)]
enum ScalarPairDifferingSign { //~ERROR: abi: ScalarPair
A(u8),
B(i8),
}
8 changes: 7 additions & 1 deletion tests/ui/layout/enum.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,11 @@ error: size: Size(16 bytes)
LL | enum UninhabitedVariantSpace {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors
error: abi: ScalarPair(Initialized { value: Int(I8, false), valid_range: 0..=1 }, Initialized { value: Int(I8, false), valid_range: 0..=255 })
--> $DIR/enum.rs:21:1
|
LL | enum ScalarPairDifferingSign {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors

Loading