Skip to content

Commit

Permalink
Auto merge of rust-lang#122048 - erikdesjardins:inbounds, r=<try>
Browse files Browse the repository at this point in the history
Use GEP inbounds for ZST and DST field offsets

ZST field offsets have been non-`inbounds` since I made [this old layout change](https://github.com/rust-lang/rust/pull/73453/files#diff-160634de1c336f2cf325ff95b312777326f1ab29fec9b9b21d5ee9aae215ecf5). Before that, they would have been `inbounds` due to using `struct_gep`. Using `inbounds` for ZSTs likely doesn't matter for performance, but I'd like to remove the special case.

DST field offsets have been non-`inbounds` since the alignment-aware DST field offset computation was first [implemented](erikdesjardins@a2557d4#diff-04fd352da30ca186fe0bb71cc81a503d1eb8a02ca17a3769e1b95981cd20964aR1188) in 1.6 (back then `GEPi()` would be used for `inbounds`), but I don't think there was any reason for it.

Split out from rust-lang#121577 / rust-lang#121665.

r? `@oli-obk`

cc `@RalfJung` -- is there some weird situation where field offsets can't be `inbounds`?

Note that it's fine for `inbounds` offsets to be one-past-the-end, so it's okay even if there's a ZST as 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. [(link)](https://llvm.org/docs/LangRef.html#getelementptr-instruction)

For rust-lang/unsafe-code-guidelines#93, zero-offset GEP is (now) always `inbounds`:

> Note that getelementptr with all-zero indices is always considered to be inbounds, even if the base pointer does not point to an allocated object. [(link)](https://llvm.org/docs/LangRef.html#getelementptr-instruction)
  • Loading branch information
bors committed Mar 6, 2024
2 parents b6d2d84 + 8ebd307 commit 54c6dee
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 9 deletions.
7 changes: 1 addition & 6 deletions compiler/rustc_codegen_ssa/src/mir/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
};
Expand Down Expand Up @@ -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 }
}
Expand Down
69 changes: 69 additions & 0 deletions tests/codegen/dst-offset.rs
Original file line number Diff line number Diff line change
@@ -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<T: ?Sized> {
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>) -> &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)
}
6 changes: 3 additions & 3 deletions tests/codegen/zst-offset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down

0 comments on commit 54c6dee

Please sign in to comment.