From 8ebd307d2a95840e193ff03aff21c87b5c643ef4 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 28 Feb 2024 00:09:24 -0500 Subject: [PATCH 1/2] use GEP inbounds for ZST and DST field offsets For the former, it's fine for `inbounds` offsets to be one-past-the-end, so it's okay even if the ZST is the last field in the layout: > The base pointer has an in bounds address of an allocated object, > which means that it points into an allocated object, or to its end. https://llvm.org/docs/LangRef.html#getelementptr-instruction For the latter, even DST fields must always be inside the layout (or to its end for ZSTs), so using inbounds is also fine there. --- compiler/rustc_codegen_ssa/src/mir/place.rs | 7 +-- tests/codegen/dst-offset.rs | 69 +++++++++++++++++++++ tests/codegen/zst-offset.rs | 6 +- 3 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 tests/codegen/dst-offset.rs diff --git a/compiler/rustc_codegen_ssa/src/mir/place.rs b/compiler/rustc_codegen_ssa/src/mir/place.rs index 725d3bf4431e4..1ec6c351e2537 100644 --- a/compiler/rustc_codegen_ssa/src/mir/place.rs +++ b/compiler/rustc_codegen_ssa/src/mir/place.rs @@ -104,10 +104,6 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { let mut simple = || { let llval = if offset.bytes() == 0 { self.llval - } else if field.is_zst() { - // FIXME(erikdesjardins): it should be fine to use inbounds for ZSTs too; - // keeping this logic for now to preserve previous behavior. - bx.ptradd(self.llval, bx.const_usize(offset.bytes())) } else { bx.inbounds_ptradd(self.llval, bx.const_usize(offset.bytes())) }; @@ -168,8 +164,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { debug!("struct_field_ptr: DST field offset: {:?}", offset); // Adjust pointer. - // FIXME(erikdesjardins): should be able to use inbounds here too. - let ptr = bx.ptradd(self.llval, offset); + let ptr = bx.inbounds_ptradd(self.llval, offset); PlaceRef { llval: ptr, llextra: self.llextra, layout: field, align: effective_field_align } } diff --git a/tests/codegen/dst-offset.rs b/tests/codegen/dst-offset.rs new file mode 100644 index 0000000000000..55212112b3d98 --- /dev/null +++ b/tests/codegen/dst-offset.rs @@ -0,0 +1,69 @@ +//! This file tests that we correctly generate GEP instructions for DST +//! field offsets. +//@ compile-flags: -C no-prepopulate-passes -Copt-level=0 + +#![crate_type = "lib"] + +use std::ptr::addr_of; + +// Hack to get the correct type for usize +// CHECK: @helper([[USIZE:i[0-9]+]] %_1) +#[no_mangle] +pub fn helper(_: usize) { +} + +struct Dst { + x: u32, + y: u8, + z: T, +} + +// CHECK: @dst_dyn_trait_offset(ptr align {{[0-9]+}} [[DATA_PTR:%.+]], ptr align {{[0-9]+}} [[VTABLE_PTR:%.+]]) +#[no_mangle] +pub fn dst_dyn_trait_offset(s: &Dst) -> &dyn Drop { +// The alignment of dyn trait is unknown, so we compute the offset based on align from the vtable. + +// CHECK: [[SIZE_PTR:%[0-9]+]] = getelementptr inbounds {{.+}} [[VTABLE_PTR]] +// CHECK: load [[USIZE]], ptr [[SIZE_PTR]] +// CHECK: [[ALIGN_PTR:%[0-9]+]] = getelementptr inbounds {{.+}} [[VTABLE_PTR]] +// CHECK: load [[USIZE]], ptr [[ALIGN_PTR]] + +// CHECK: getelementptr inbounds i8, ptr [[DATA_PTR]] +// CHECK-NEXT: insertvalue +// CHECK-NEXT: insertvalue +// CHECK-NEXT: ret + &s.z +} + +// CHECK-LABEL: @dst_slice_offset +#[no_mangle] +pub fn dst_slice_offset(s: &Dst<[u16]>) -> &[u16] { +// The alignment of [u16] is known, so we generate a GEP directly. + +// CHECK: start: +// CHECK-NEXT: getelementptr inbounds i8, {{.+}}, [[USIZE]] 6 +// CHECK-NEXT: insertvalue +// CHECK-NEXT: insertvalue +// CHECK-NEXT: ret + &s.z +} + +#[repr(packed)] +struct PackedDstSlice { + x: u32, + y: u8, + z: [u16], +} + +// CHECK-LABEL: @packed_dst_slice_offset +#[no_mangle] +pub fn packed_dst_slice_offset(s: &PackedDstSlice) -> *const [u16] { +// The alignment of [u16] is known, so we generate a GEP directly. + +// CHECK: start: +// CHECK-NEXT: getelementptr inbounds i8, {{.+}}, [[USIZE]] 5 +// CHECK-NEXT: insertvalue +// CHECK-NEXT: insertvalue +// CHECK-NEXT: ret + addr_of!(s.z) +} diff --git a/tests/codegen/zst-offset.rs b/tests/codegen/zst-offset.rs index 65d9cf39c4c61..b623d492d9d69 100644 --- a/tests/codegen/zst-offset.rs +++ b/tests/codegen/zst-offset.rs @@ -13,7 +13,7 @@ pub fn helper(_: usize) { // CHECK-LABEL: @scalar_layout #[no_mangle] pub fn scalar_layout(s: &(u64, ())) { -// CHECK: getelementptr i8, {{.+}}, [[USIZE]] 8 +// CHECK: getelementptr inbounds i8, {{.+}}, [[USIZE]] 8 let x = &s.1; witness(&x); // keep variable in an alloca } @@ -22,7 +22,7 @@ pub fn scalar_layout(s: &(u64, ())) { // CHECK-LABEL: @scalarpair_layout #[no_mangle] pub fn scalarpair_layout(s: &(u64, u32, ())) { -// CHECK: getelementptr i8, {{.+}}, [[USIZE]] 12 +// CHECK: getelementptr inbounds i8, {{.+}}, [[USIZE]] 12 let x = &s.2; witness(&x); // keep variable in an alloca } @@ -34,7 +34,7 @@ pub struct U64x4(u64, u64, u64, u64); // CHECK-LABEL: @vector_layout #[no_mangle] pub fn vector_layout(s: &(U64x4, ())) { -// CHECK: getelementptr i8, {{.+}}, [[USIZE]] 32 +// CHECK: getelementptr inbounds i8, {{.+}}, [[USIZE]] 32 let x = &s.1; witness(&x); // keep variable in an alloca } From e349900339fc5ff512828aee9fddf87995e5e347 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Wed, 6 Mar 2024 19:53:45 -0500 Subject: [PATCH 2/2] add test for extern type --- tests/codegen/dst-offset.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/codegen/dst-offset.rs b/tests/codegen/dst-offset.rs index 55212112b3d98..f0157e5a10646 100644 --- a/tests/codegen/dst-offset.rs +++ b/tests/codegen/dst-offset.rs @@ -4,6 +4,8 @@ #![crate_type = "lib"] +#![feature(extern_types)] + use std::ptr::addr_of; // Hack to get the correct type for usize @@ -67,3 +69,16 @@ pub fn packed_dst_slice_offset(s: &PackedDstSlice) -> *const [u16] { // CHECK-NEXT: ret addr_of!(s.z) } + +extern { + pub type Extern; +} + +// CHECK-LABEL: @dst_extern +#[no_mangle] +pub fn dst_extern(s: &Dst) -> &Extern { +// Computing the alignment of an extern type is currently unsupported and just panics. + +// CHECK: call void @{{.+}}panic + &s.z +}