From 4dabbcb23b0dc605cdb4aa6f3499d0f053f7b600 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Tue, 27 Feb 2024 00:09:12 -0500 Subject: [PATCH 1/5] allow using scalarpair with a common prim of ptr/ptr-sized-int --- compiler/rustc_abi/src/layout.rs | 41 ++++++- tests/codegen/common_prim_int_ptr.rs | 43 ++++++++ tests/codegen/try_question_mark_nop.rs | 104 ++++++++++++++++-- tests/ui/layout/enum-scalar-pair-int-ptr.rs | 24 ++++ .../ui/layout/enum-scalar-pair-int-ptr.stderr | 14 +++ tests/ui/layout/enum.rs | 6 + tests/ui/layout/enum.stderr | 8 +- 7 files changed, 221 insertions(+), 19 deletions(-) create mode 100644 tests/codegen/common_prim_int_ptr.rs create mode 100644 tests/ui/layout/enum-scalar-pair-int-ptr.rs create mode 100644 tests/ui/layout/enum-scalar-pair-int-ptr.stderr diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index 28e148bddb220..d023c869e8b64 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -813,15 +813,44 @@ 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_common_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_common_prim = match (old_common_prim, prim) { + // Allow all identical primitives. + (x, y) if x == y => Some(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 => Some(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) => + { + Some(p) + } + _ => None, + }; + match new_common_prim { + Some(new_prim) => { + // Primitives are compatible. + // We may be updating the primitive here, for example from int->ptr. + common_prim = Some((new_prim, common_offset)); + } + None => { + // Primitives are incompatible. + common_prim = None; + break; + } + } } else { common_prim = Some((prim, offset)); } diff --git a/tests/codegen/common_prim_int_ptr.rs b/tests/codegen/common_prim_int_ptr.rs new file mode 100644 index 0000000000000..9b798d495d4f0 --- /dev/null +++ b/tests/codegen/common_prim_int_ptr.rs @@ -0,0 +1,43 @@ +//@ compile-flags: -O + +#![crate_type = "lib"] + +// Tests that codegen works properly when enums like `Result>` +// 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> { + // CHECK: start: + // CHECK-NEXT: inttoptr i{{[0-9]+}} %x to ptr + // CHECK-NEXT: insertvalue + // CHECK-NEXT: ret { i{{[0-9]+}}, ptr } + Ok(x) +} + +// CHECK-LABEL: @insert_box +#[no_mangle] +pub fn insert_box(x: Box<()>) -> Result> { + // 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 { + // CHECK: start: + // CHECK-NEXT: ptrtoint + // CHECK-NEXT: ret + x.unwrap_unchecked() +} + +// CHECK-LABEL: @extract_box +#[no_mangle] +pub unsafe fn extract_box(x: Result>) -> Box<()> { + // CHECK: start: + // CHECK-NEXT: ret ptr + x.unwrap_err_unchecked() +} diff --git a/tests/codegen/try_question_mark_nop.rs b/tests/codegen/try_question_mark_nop.rs index 58cd6ff233a1e..f6cdf95520975 100644 --- a/tests/codegen/try_question_mark_nop.rs +++ b/tests/codegen/try_question_mark_nop.rs @@ -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 -// . -// 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) -> Option { + // 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) -> Option { + // 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) -> Result { - // 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 { Ok(x) => Ok(x), Err(x) => Err(x), @@ -24,8 +48,60 @@ pub fn result_nop_match_32(x: Result) -> Result { // CHECK-LABEL: @result_nop_traits_32 #[no_mangle] pub fn result_nop_traits_32(x: Result) -> Result { - // 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) -> Result { + // 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) -> Result { + // 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>) -> Result> { + // 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>) -> Result> { + // CHECK: start: + // CHECK-NEXT: insertvalue { i{{[0-9]+}}, ptr } + // CHECK-NEXT: insertvalue { i{{[0-9]+}}, ptr } + // CHECK-NEXT: ret try { x? } @@ -34,8 +110,10 @@ pub fn result_nop_traits_32(x: Result) -> Result { // CHECK-LABEL: @control_flow_nop_match_32 #[no_mangle] pub fn control_flow_nop_match_32(x: ControlFlow) -> ControlFlow { - // 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), @@ -45,8 +123,10 @@ pub fn control_flow_nop_match_32(x: ControlFlow) -> ControlFlow) -> ControlFlow { - // 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? } diff --git a/tests/ui/layout/enum-scalar-pair-int-ptr.rs b/tests/ui/layout/enum-scalar-pair-int-ptr.rs new file mode 100644 index 0000000000000..a1aec094d8025 --- /dev/null +++ b/tests/ui/layout/enum-scalar-pair-int-ptr.rs @@ -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<()>), +} diff --git a/tests/ui/layout/enum-scalar-pair-int-ptr.stderr b/tests/ui/layout/enum-scalar-pair-int-ptr.stderr new file mode 100644 index 0000000000000..b25eda628cd6a --- /dev/null +++ b/tests/ui/layout/enum-scalar-pair-int-ptr.stderr @@ -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 + diff --git a/tests/ui/layout/enum.rs b/tests/ui/layout/enum.rs index bde8450b9d569..5710634cf6b1f 100644 --- a/tests/ui/layout/enum.rs +++ b/tests/ui/layout/enum.rs @@ -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(u64), + B(i64), +} diff --git a/tests/ui/layout/enum.stderr b/tests/ui/layout/enum.stderr index d6bc7780ce206..189c4225ec54d 100644 --- a/tests/ui/layout/enum.stderr +++ b/tests/ui/layout/enum.stderr @@ -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(I64, false), valid_range: 0..=1 }, Initialized { value: Int(I64, false), valid_range: 0..=18446744073709551615 }) + --> $DIR/enum.rs:21:1 + | +LL | enum ScalarPairDifferingSign { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 3 previous errors From f574c651476a43dd7ca92ef859fb501d18a8c3ff Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 28 Feb 2024 17:55:04 -0500 Subject: [PATCH 2/5] simplify common prim computation --- compiler/rustc_abi/src/layout.rs | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_abi/src/layout.rs b/compiler/rustc_abi/src/layout.rs index d023c869e8b64..a476d83d4c7ff 100644 --- a/compiler/rustc_abi/src/layout.rs +++ b/compiler/rustc_abi/src/layout.rs @@ -813,7 +813,7 @@ where break; } }; - if let Some((old_common_prim, common_offset)) = common_prim { + if let Some((old_prim, common_offset)) = common_prim { // All variants must be at the same offset if offset != common_offset { common_prim = None; @@ -822,12 +822,12 @@ where // This is pretty conservative. We could go fancier // by realising that (u8, u8) could just cohabit with // u16 or even u32. - let new_common_prim = match (old_common_prim, prim) { + let new_prim = match (old_prim, prim) { // Allow all identical primitives. - (x, y) if x == y => Some(x), + (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 => Some(p), + (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. @@ -835,22 +835,15 @@ where | (i @ Primitive::Int(..), p @ Primitive::Pointer(_)) if p.size(dl) == i.size(dl) && p.align(dl) == i.align(dl) => { - Some(p) + p } - _ => None, - }; - match new_common_prim { - Some(new_prim) => { - // Primitives are compatible. - // We may be updating the primitive here, for example from int->ptr. - common_prim = Some((new_prim, common_offset)); - } - None => { - // Primitives are incompatible. + _ => { 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)); } From 8e40b17b6bc57e9a905b263b79e254895252ea49 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 28 Feb 2024 18:48:14 -0500 Subject: [PATCH 3/5] fix test failure due to differing u64 alignment on different targets --- tests/ui/layout/enum.rs | 4 ++-- tests/ui/layout/enum.stderr | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/ui/layout/enum.rs b/tests/ui/layout/enum.rs index 5710634cf6b1f..e0a7fc328df30 100644 --- a/tests/ui/layout/enum.rs +++ b/tests/ui/layout/enum.rs @@ -19,6 +19,6 @@ enum UninhabitedVariantSpace { //~ERROR: size: Size(16 bytes) #[rustc_layout(abi)] enum ScalarPairDifferingSign { //~ERROR: abi: ScalarPair - A(u64), - B(i64), + A(u8), + B(i8), } diff --git a/tests/ui/layout/enum.stderr b/tests/ui/layout/enum.stderr index 189c4225ec54d..7f0b38d0a07ce 100644 --- a/tests/ui/layout/enum.stderr +++ b/tests/ui/layout/enum.stderr @@ -10,7 +10,7 @@ error: size: Size(16 bytes) LL | enum UninhabitedVariantSpace { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: abi: ScalarPair(Initialized { value: Int(I64, false), valid_range: 0..=1 }, Initialized { value: Int(I64, false), valid_range: 0..=18446744073709551615 }) +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 { From 5ccada66a21e2fff8691b97189f127ecfd42ea26 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 6 Mar 2024 19:36:09 -0500 Subject: [PATCH 4/5] make check lines for int/ptr common prim test more permissive It seems that LLVM 17 doesn't fully optimize out unwrap_unchecked. We can just loosen the check lines to account for this, since we don't really care about the exact instructions, we just want to make sure that inttoptr/ptrtoint aren't used for Box. --- tests/codegen/common_prim_int_ptr.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/codegen/common_prim_int_ptr.rs b/tests/codegen/common_prim_int_ptr.rs index 9b798d495d4f0..666ccc5a2cff3 100644 --- a/tests/codegen/common_prim_int_ptr.rs +++ b/tests/codegen/common_prim_int_ptr.rs @@ -28,16 +28,16 @@ pub fn insert_box(x: Box<()>) -> Result> { // CHECK-LABEL: @extract_int #[no_mangle] pub unsafe fn extract_int(x: Result>) -> usize { - // CHECK: start: - // CHECK-NEXT: ptrtoint - // CHECK-NEXT: ret + // CHECK: ptrtoint x.unwrap_unchecked() } // CHECK-LABEL: @extract_box #[no_mangle] pub unsafe fn extract_box(x: Result>) -> Box<()> { - // CHECK: start: - // CHECK-NEXT: ret ptr + // CHECK-NOT: ptrtoint + // CHECK-NOT: inttoptr + // CHECK-NOT: load + // CHECK-NOT: store x.unwrap_err_unchecked() } From 9f55200a42f599549fee0f003b27888d03604861 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 13 Mar 2024 01:17:15 -0400 Subject: [PATCH 5/5] refine common_prim test Co-authored-by: Scott McMurray --- tests/codegen/common_prim_int_ptr.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/tests/codegen/common_prim_int_ptr.rs b/tests/codegen/common_prim_int_ptr.rs index 666ccc5a2cff3..87fa89abb8667 100644 --- a/tests/codegen/common_prim_int_ptr.rs +++ b/tests/codegen/common_prim_int_ptr.rs @@ -1,6 +1,7 @@ //@ compile-flags: -O #![crate_type = "lib"] +#![feature(core_intrinsics)] // Tests that codegen works properly when enums like `Result>` // are represented as `{ u64, ptr }`, i.e., for `Ok(123)`, `123` is stored @@ -26,18 +27,25 @@ pub fn insert_box(x: Box<()>) -> Result> { } // CHECK-LABEL: @extract_int +// CHECK-NOT: nonnull +// CHECK-SAME: (i{{[0-9]+}} {{[^,]+}} [[DISCRIMINANT:%[0-9]+]], ptr {{[^,]+}} [[PAYLOAD:%[0-9]+]]) #[no_mangle] pub unsafe fn extract_int(x: Result>) -> usize { - // CHECK: ptrtoint - x.unwrap_unchecked() + // CHECK: [[TEMP:%.+]] = ptrtoint ptr [[PAYLOAD]] to [[USIZE:i[0-9]+]] + // CHECK: ret [[USIZE]] [[TEMP]] + match x { + Ok(v) => v, + Err(_) => std::intrinsics::unreachable(), + } } // CHECK-LABEL: @extract_box +// CHECK-SAME: (i{{[0-9]+}} {{[^,]+}} [[DISCRIMINANT:%[0-9]+]], ptr {{[^,]+}} [[PAYLOAD:%[0-9]+]]) #[no_mangle] -pub unsafe fn extract_box(x: Result>) -> Box<()> { - // CHECK-NOT: ptrtoint - // CHECK-NOT: inttoptr - // CHECK-NOT: load - // CHECK-NOT: store - x.unwrap_err_unchecked() +pub unsafe fn extract_box(x: Result>) -> Box { + // CHECK: ret ptr [[PAYLOAD]] + match x { + Ok(_) => std::intrinsics::unreachable(), + Err(e) => e, + } }