From 123015e7224fda0e9600962aa23202ac322271bc Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Fri, 23 Feb 2024 23:23:35 -0500 Subject: [PATCH 1/5] always use gep inbounds i8 (ptradd) for field offsets --- compiler/rustc_codegen_gcc/src/type_of.rs | 23 ------------- compiler/rustc_codegen_llvm/src/type_.rs | 3 -- compiler/rustc_codegen_ssa/src/mir/place.rs | 34 +++---------------- .../rustc_codegen_ssa/src/traits/type_.rs | 1 - tests/codegen/align-struct.rs | 1 - tests/codegen/i128-x86-align.rs | 4 +-- .../issues/issue-105386-ub-in-debuginfo.rs | 4 +-- tests/codegen/zst-offset.rs | 4 +-- 8 files changed, 11 insertions(+), 63 deletions(-) diff --git a/compiler/rustc_codegen_gcc/src/type_of.rs b/compiler/rustc_codegen_gcc/src/type_of.rs index 25149b8020162..e327051ca1d58 100644 --- a/compiler/rustc_codegen_gcc/src/type_of.rs +++ b/compiler/rustc_codegen_gcc/src/type_of.rs @@ -151,7 +151,6 @@ pub trait LayoutGccExt<'tcx> { fn immediate_gcc_type<'gcc>(&self, cx: &CodegenCx<'gcc, 'tcx>) -> Type<'gcc>; fn scalar_gcc_type_at<'gcc>(&self, cx: &CodegenCx<'gcc, 'tcx>, scalar: &abi::Scalar, offset: Size) -> Type<'gcc>; fn scalar_pair_element_gcc_type<'gcc>(&self, cx: &CodegenCx<'gcc, 'tcx>, index: usize) -> Type<'gcc>; - fn gcc_field_index(&self, index: usize) -> u64; fn pointee_info_at<'gcc>(&self, cx: &CodegenCx<'gcc, 'tcx>, offset: Size) -> Option; } @@ -304,24 +303,6 @@ impl<'tcx> LayoutGccExt<'tcx> for TyAndLayout<'tcx> { self.scalar_gcc_type_at(cx, scalar, offset) } - fn gcc_field_index(&self, index: usize) -> u64 { - match self.abi { - Abi::Scalar(_) | Abi::ScalarPair(..) => { - bug!("TyAndLayout::gcc_field_index({:?}): not applicable", self) - } - _ => {} - } - match self.fields { - FieldsShape::Primitive | FieldsShape::Union(_) => { - bug!("TyAndLayout::gcc_field_index({:?}): not applicable", self) - } - - FieldsShape::Array { .. } => index as u64, - - FieldsShape::Arbitrary { .. } => 1 + (self.fields.memory_index(index) as u64) * 2, - } - } - fn pointee_info_at<'a>(&self, cx: &CodegenCx<'a, 'tcx>, offset: Size) -> Option { if let Some(&pointee) = cx.pointee_infos.borrow().get(&(self.ty, offset)) { return pointee; @@ -351,10 +332,6 @@ impl<'gcc, 'tcx> LayoutTypeMethods<'tcx> for CodegenCx<'gcc, 'tcx> { layout.is_gcc_scalar_pair() } - fn backend_field_index(&self, layout: TyAndLayout<'tcx>, index: usize) -> u64 { - layout.gcc_field_index(index) - } - fn scalar_pair_element_backend_type(&self, layout: TyAndLayout<'tcx>, index: usize, _immediate: bool) -> Type<'gcc> { layout.scalar_pair_element_gcc_type(self, index) } diff --git a/compiler/rustc_codegen_llvm/src/type_.rs b/compiler/rustc_codegen_llvm/src/type_.rs index 447c4ed1f0c6f..c0245de36f714 100644 --- a/compiler/rustc_codegen_llvm/src/type_.rs +++ b/compiler/rustc_codegen_llvm/src/type_.rs @@ -250,9 +250,6 @@ impl<'ll, 'tcx> LayoutTypeMethods<'tcx> for CodegenCx<'ll, 'tcx> { fn is_backend_scalar_pair(&self, layout: TyAndLayout<'tcx>) -> bool { layout.is_llvm_scalar_pair() } - fn backend_field_index(&self, layout: TyAndLayout<'tcx>, index: usize) -> u64 { - layout.llvm_field_index(self, index) - } fn scalar_pair_element_backend_type( &self, layout: TyAndLayout<'tcx>, diff --git a/compiler/rustc_codegen_ssa/src/mir/place.rs b/compiler/rustc_codegen_ssa/src/mir/place.rs index 73c08e2ca61ec..0ce890dca3204 100644 --- a/compiler/rustc_codegen_ssa/src/mir/place.rs +++ b/compiler/rustc_codegen_ssa/src/mir/place.rs @@ -9,7 +9,7 @@ use rustc_middle::mir; use rustc_middle::mir::tcx::PlaceTy; use rustc_middle::ty::layout::{HasTyCtxt, LayoutOf, TyAndLayout}; use rustc_middle::ty::{self, Ty}; -use rustc_target::abi::{Abi, Align, FieldsShape, Int, Pointer, TagEncoding}; +use rustc_target::abi::{Align, FieldsShape, Int, Pointer, TagEncoding}; use rustc_target::abi::{VariantIdx, Variants}; #[derive(Copy, Clone, Debug)] @@ -102,34 +102,10 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { // `simple` is called when we don't need to adjust the offset to // the dynamic alignment of the field. let mut simple = || { - let llval = match self.layout.abi { - _ if offset.bytes() == 0 => { - // Unions and newtypes only use an offset of 0. - // Also handles the first field of Scalar, ScalarPair, and Vector layouts. - self.llval - } - Abi::ScalarPair(..) => { - // FIXME(nikic): Generate this for all ABIs. - bx.inbounds_gep(bx.type_i8(), self.llval, &[bx.const_usize(offset.bytes())]) - } - Abi::Scalar(_) | Abi::Vector { .. } if field.is_zst() => { - // ZST fields (even some that require alignment) are not included in Scalar, - // ScalarPair, and Vector layouts, so manually offset the pointer. - bx.gep(bx.cx().type_i8(), self.llval, &[bx.const_usize(offset.bytes())]) - } - Abi::Scalar(_) => { - // All fields of Scalar layouts must have been handled by this point. - // Vector layouts have additional fields for each element of the vector, so don't panic in that case. - bug!( - "offset of non-ZST field `{:?}` does not match layout `{:#?}`", - field, - self.layout - ); - } - _ => { - let ty = bx.backend_type(self.layout); - bx.struct_gep(ty, self.llval, bx.cx().backend_field_index(self.layout, ix)) - } + let llval = if offset.bytes() == 0 { + self.llval + } else { + bx.inbounds_gep(bx.type_i8(), self.llval, &[bx.const_usize(offset.bytes())]) }; PlaceRef { llval, diff --git a/compiler/rustc_codegen_ssa/src/traits/type_.rs b/compiler/rustc_codegen_ssa/src/traits/type_.rs index b1fde8e4d8638..fb38a92033201 100644 --- a/compiler/rustc_codegen_ssa/src/traits/type_.rs +++ b/compiler/rustc_codegen_ssa/src/traits/type_.rs @@ -111,7 +111,6 @@ pub trait LayoutTypeMethods<'tcx>: Backend<'tcx> { fn immediate_backend_type(&self, layout: TyAndLayout<'tcx>) -> Self::Type; fn is_backend_immediate(&self, layout: TyAndLayout<'tcx>) -> bool; fn is_backend_scalar_pair(&self, layout: TyAndLayout<'tcx>) -> bool; - fn backend_field_index(&self, layout: TyAndLayout<'tcx>, index: usize) -> u64; fn scalar_pair_element_backend_type( &self, layout: TyAndLayout<'tcx>, diff --git a/tests/codegen/align-struct.rs b/tests/codegen/align-struct.rs index 34475a3852b70..31859152830a5 100644 --- a/tests/codegen/align-struct.rs +++ b/tests/codegen/align-struct.rs @@ -26,7 +26,6 @@ pub enum Enum64 { B(i32), } // CHECK: %Enum64 = type { i32, [31 x i32] } -// CHECK: %"Enum64::A" = type { [8 x i64], %Align64 } // CHECK-LABEL: @align64 #[no_mangle] diff --git a/tests/codegen/i128-x86-align.rs b/tests/codegen/i128-x86-align.rs index 9cc5c3d3ed7b1..b2e0c294c39d6 100644 --- a/tests/codegen/i128-x86-align.rs +++ b/tests/codegen/i128-x86-align.rs @@ -94,9 +94,9 @@ pub fn store_struct(x: &mut Struct) { // CHECK-SAME: align 16 dereferenceable(32) %x // CHECK: [[TMP:%.*]] = alloca %Struct, align 16 // CHECK: store i32 1, ptr [[TMP]], align 16 - // CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds %Struct, ptr [[TMP]], i32 0, i32 1 + // CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds i8, ptr [[TMP]], i64 4 // CHECK-NEXT: store i32 2, ptr [[GEP1]], align 4 - // CHECK-NEXT: [[GEP2:%.*]] = getelementptr inbounds %Struct, ptr [[TMP]], i32 0, i32 3 + // CHECK-NEXT: [[GEP2:%.*]] = getelementptr inbounds i8, ptr [[TMP]], i64 16 // CHECK-NEXT: store i128 3, ptr [[GEP2]], align 16 // CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 16 %x, ptr align 16 [[TMP]], i64 32, i1 false) *x = Struct { a: 1, b: 2, c: 3 }; diff --git a/tests/codegen/issues/issue-105386-ub-in-debuginfo.rs b/tests/codegen/issues/issue-105386-ub-in-debuginfo.rs index 476db7c135850..0bd43dc50b21a 100644 --- a/tests/codegen/issues/issue-105386-ub-in-debuginfo.rs +++ b/tests/codegen/issues/issue-105386-ub-in-debuginfo.rs @@ -16,8 +16,8 @@ pub fn outer_function(x: S, y: S) -> usize { // when generating debuginfo. // CHECK-LABEL: @outer_function // CHECK: [[spill:%.*]] = alloca %"{closure@{{.*.rs}}:9:23: 9:25}" -// CHECK-NOT: [[ptr_tmp:%.*]] = getelementptr inbounds %"{closure@{{.*.rs}}:9:23: 9:25}", ptr [[spill]] +// CHECK-NOT: [[ptr_tmp:%.*]] = getelementptr inbounds i8, ptr [[spill]] // CHECK-NOT: [[load:%.*]] = load ptr, ptr // CHECK: call void @llvm.lifetime.start{{.*}}({{.*}}, ptr [[spill]]) -// CHECK: [[inner:%.*]] = getelementptr inbounds %"{{.*}}", ptr [[spill]] +// CHECK: [[inner:%.*]] = getelementptr inbounds i8, ptr [[spill]] // CHECK: call void @llvm.memcpy{{.*}}(ptr {{align .*}} [[inner]], ptr {{align .*}} %x diff --git a/tests/codegen/zst-offset.rs b/tests/codegen/zst-offset.rs index ad996d8ae1b6d..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 } @@ -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 beed25be9a2a9e1e73ca79210da57d294c27f925 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Sat, 24 Feb 2024 01:46:30 -0500 Subject: [PATCH 2/5] remove struct_gep, use manual layout calculations for va_arg --- compiler/rustc_codegen_gcc/src/builder.rs | 38 ++------ compiler/rustc_codegen_llvm/src/builder.rs | 5 - compiler/rustc_codegen_llvm/src/llvm/ffi.rs | 7 -- compiler/rustc_codegen_llvm/src/type_of.rs | 37 -------- compiler/rustc_codegen_llvm/src/va_arg.rs | 94 ++++++++++++------- .../rustc_codegen_ssa/src/traits/builder.rs | 1 - 6 files changed, 71 insertions(+), 111 deletions(-) diff --git a/compiler/rustc_codegen_gcc/src/builder.rs b/compiler/rustc_codegen_gcc/src/builder.rs index 7e2139866f49e..bae0cc3655df6 100644 --- a/compiler/rustc_codegen_gcc/src/builder.rs +++ b/compiler/rustc_codegen_gcc/src/builder.rs @@ -834,10 +834,17 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { } else if let abi::Abi::ScalarPair(ref a, ref b) = place.layout.abi { let b_offset = a.size(self).align_to(b.align(self).abi); - let pair_type = place.layout.gcc_type(self); let mut load = |i, scalar: &abi::Scalar, align| { - let llptr = self.struct_gep(pair_type, place.llval, i as u64); + let llptr = if i == 0 { + place.llval + } else { + self.inbounds_gep( + self.type_i8(), + place.llval, + &[self.const_usize(b_offset.bytes())], + ) + }; let llty = place.layout.scalar_pair_element_gcc_type(self, i); let load = self.load(llty, llptr, align); scalar_load_metadata(self, load, scalar); @@ -971,33 +978,6 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { result.get_address(None) } - fn struct_gep(&mut self, value_type: Type<'gcc>, ptr: RValue<'gcc>, idx: u64) -> RValue<'gcc> { - // FIXME(antoyo): it would be better if the API only called this on struct, not on arrays. - assert_eq!(idx as usize as u64, idx); - let value = ptr.dereference(None).to_rvalue(); - - if value_type.dyncast_array().is_some() { - let index = self.context.new_rvalue_from_long(self.u64_type, i64::try_from(idx).expect("i64::try_from")); - let element = self.context.new_array_access(None, value, index); - element.get_address(None) - } - else if let Some(vector_type) = value_type.dyncast_vector() { - let array_type = vector_type.get_element_type().make_pointer(); - let array = self.bitcast(ptr, array_type); - let index = self.context.new_rvalue_from_long(self.u64_type, i64::try_from(idx).expect("i64::try_from")); - let element = self.context.new_array_access(None, array, index); - element.get_address(None) - } - else if let Some(struct_type) = value_type.is_struct() { - // NOTE: due to opaque pointers now being used, we need to bitcast here. - let ptr = self.bitcast_if_needed(ptr, value_type.make_pointer()); - ptr.dereference_field(None, struct_type.get_field(idx as i32)).get_address(None) - } - else { - panic!("Unexpected type {:?}", value_type); - } - } - /* Casts */ fn trunc(&mut self, value: RValue<'gcc>, dest_ty: Type<'gcc>) -> RValue<'gcc> { // TODO(antoyo): check that it indeed truncate the value. diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 8cab2a3f27c1b..146fa591926c0 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -778,11 +778,6 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { } } - fn struct_gep(&mut self, ty: &'ll Type, ptr: &'ll Value, idx: u64) -> &'ll Value { - assert_eq!(idx as c_uint as u64, idx); - unsafe { llvm::LLVMBuildStructGEP2(self.llbuilder, ty, ptr, idx as c_uint, UNNAMED) } - } - /* Casts */ fn trunc(&mut self, val: &'ll Value, dest_ty: &'ll Type) -> &'ll Value { unsafe { llvm::LLVMBuildTrunc(self.llbuilder, val, dest_ty, UNNAMED) } diff --git a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs index 70fc7a66bcdc7..4472cc2264bb4 100644 --- a/compiler/rustc_codegen_llvm/src/llvm/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/llvm/ffi.rs @@ -1301,13 +1301,6 @@ extern "C" { NumIndices: c_uint, Name: *const c_char, ) -> &'a Value; - pub fn LLVMBuildStructGEP2<'a>( - B: &Builder<'a>, - Ty: &'a Type, - Pointer: &'a Value, - Idx: c_uint, - Name: *const c_char, - ) -> &'a Value; // Casts pub fn LLVMBuildTrunc<'a>( diff --git a/compiler/rustc_codegen_llvm/src/type_of.rs b/compiler/rustc_codegen_llvm/src/type_of.rs index 219c702531141..6520104fc6d8a 100644 --- a/compiler/rustc_codegen_llvm/src/type_of.rs +++ b/compiler/rustc_codegen_llvm/src/type_of.rs @@ -174,7 +174,6 @@ pub trait LayoutLlvmExt<'tcx> { index: usize, immediate: bool, ) -> &'a Type; - fn llvm_field_index<'a>(&self, cx: &CodegenCx<'a, 'tcx>, index: usize) -> u64; fn scalar_copy_llvm_type<'a>(&self, cx: &CodegenCx<'a, 'tcx>) -> Option<&'a Type>; } @@ -324,42 +323,6 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyAndLayout<'tcx> { self.scalar_llvm_type_at(cx, scalar) } - fn llvm_field_index<'a>(&self, cx: &CodegenCx<'a, 'tcx>, index: usize) -> u64 { - match self.abi { - Abi::Scalar(_) | Abi::ScalarPair(..) => { - bug!("TyAndLayout::llvm_field_index({:?}): not applicable", self) - } - _ => {} - } - match self.fields { - FieldsShape::Primitive | FieldsShape::Union(_) => { - bug!("TyAndLayout::llvm_field_index({:?}): not applicable", self) - } - - FieldsShape::Array { .. } => index as u64, - - FieldsShape::Arbitrary { .. } => { - let variant_index = match self.variants { - Variants::Single { index } => Some(index), - _ => None, - }; - - // Look up llvm field if indexes do not match memory order due to padding. If - // `field_remapping` is `None` no padding was used and the llvm field index - // matches the memory index. - match cx.type_lowering.borrow().get(&(self.ty, variant_index)) { - Some(TypeLowering { field_remapping: Some(ref remap), .. }) => { - remap[index] as u64 - } - Some(_) => self.fields.memory_index(index) as u64, - None => { - bug!("TyAndLayout::llvm_field_index({:?}): type info not found", self) - } - } - } - } - } - fn scalar_copy_llvm_type<'a>(&self, cx: &CodegenCx<'a, 'tcx>) -> Option<&'a Type> { debug_assert!(self.is_sized()); diff --git a/compiler/rustc_codegen_llvm/src/va_arg.rs b/compiler/rustc_codegen_llvm/src/va_arg.rs index 172c66a7af13c..2d6cd51aead75 100644 --- a/compiler/rustc_codegen_llvm/src/va_arg.rs +++ b/compiler/rustc_codegen_llvm/src/va_arg.rs @@ -89,11 +89,35 @@ fn emit_aapcs_va_arg<'ll, 'tcx>( list: OperandRef<'tcx, &'ll Value>, target_ty: Ty<'tcx>, ) -> &'ll Value { + let dl = bx.cx.data_layout(); + // Implementation of the AAPCS64 calling convention for va_args see // https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst + // + // typedef struct va_list { + // void * stack; // next stack param + // void * gr_top; // end of GP arg reg save area + // void * vr_top; // end of FP/SIMD arg reg save area + // int gr_offs; // offset from gr_top to next GP register arg + // int vr_offs; // offset from vr_top to next FP/SIMD register arg + // } va_list; let va_list_addr = list.immediate(); - let va_list_layout = list.deref(bx.cx).layout; - let va_list_ty = va_list_layout.llvm_type(bx); + + // There is no padding between fields since `void*` is size=8 align=8, `int` is size=4 align=4. + // See https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst + // Table 1, Byte size and byte alignment of fundamental data types + // Table 3, Mapping of C & C++ built-in data types + let ptr_offset = 8; + let i32_offset = 4; + let gr_top = bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(ptr_offset)]); + let vr_top = bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(2 * ptr_offset)]); + let gr_offs = bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(3 * ptr_offset)]); + let vr_offs = bx.inbounds_gep( + bx.type_i8(), + va_list_addr, + &[bx.cx.const_usize(3 * ptr_offset + i32_offset)], + ); + let layout = bx.cx.layout_of(target_ty); let maybe_reg = bx.append_sibling_block("va_arg.maybe_reg"); @@ -104,16 +128,12 @@ fn emit_aapcs_va_arg<'ll, 'tcx>( let offset_align = Align::from_bytes(4).unwrap(); let gr_type = target_ty.is_any_ptr() || target_ty.is_integral(); - let (reg_off, reg_top_index, slot_size) = if gr_type { - let gr_offs = - bx.struct_gep(va_list_ty, va_list_addr, va_list_layout.llvm_field_index(bx.cx, 3)); + let (reg_off, reg_top, slot_size) = if gr_type { let nreg = (layout.size.bytes() + 7) / 8; - (gr_offs, va_list_layout.llvm_field_index(bx.cx, 1), nreg * 8) + (gr_offs, gr_top, nreg * 8) } else { - let vr_off = - bx.struct_gep(va_list_ty, va_list_addr, va_list_layout.llvm_field_index(bx.cx, 4)); let nreg = (layout.size.bytes() + 15) / 16; - (vr_off, va_list_layout.llvm_field_index(bx.cx, 2), nreg * 16) + (vr_offs, vr_top, nreg * 16) }; // if the offset >= 0 then the value will be on the stack @@ -141,8 +161,7 @@ fn emit_aapcs_va_arg<'ll, 'tcx>( bx.switch_to_block(in_reg); let top_type = bx.type_ptr(); - let top = bx.struct_gep(va_list_ty, va_list_addr, reg_top_index); - let top = bx.load(top_type, top, bx.tcx().data_layout.pointer_align.abi); + let top = bx.load(top_type, reg_top, dl.pointer_align.abi); // reg_value = *(@top + reg_off_v); let mut reg_addr = bx.gep(bx.type_i8(), top, &[reg_off_v]); @@ -173,11 +192,33 @@ fn emit_s390x_va_arg<'ll, 'tcx>( list: OperandRef<'tcx, &'ll Value>, target_ty: Ty<'tcx>, ) -> &'ll Value { + let dl = bx.cx.data_layout(); + // Implementation of the s390x ELF ABI calling convention for va_args see // https://github.com/IBM/s390x-abi (chapter 1.2.4) + // + // typedef struct __va_list_tag { + // long __gpr; + // long __fpr; + // void *__overflow_arg_area; + // void *__reg_save_area; + // } va_list[1]; let va_list_addr = list.immediate(); - let va_list_layout = list.deref(bx.cx).layout; - let va_list_ty = va_list_layout.llvm_type(bx); + + // There is no padding between fields since `long` and `void*` both have size=8 align=8. + // https://github.com/IBM/s390x-abi (Table 1.1.: Scalar types) + let i64_offset = 8; + let ptr_offset = 8; + let gpr = va_list_addr; + let fpr = bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(i64_offset)]); + let overflow_arg_area = + bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(2 * i64_offset)]); + let reg_save_area = bx.inbounds_gep( + bx.type_i8(), + va_list_addr, + &[bx.cx.const_usize(2 * i64_offset + ptr_offset)], + ); + let layout = bx.cx.layout_of(target_ty); let in_reg = bx.append_sibling_block("va_arg.in_reg"); @@ -192,15 +233,10 @@ fn emit_s390x_va_arg<'ll, 'tcx>( let padding = padded_size - unpadded_size; let gpr_type = indirect || !layout.is_single_fp_element(bx.cx); - let (max_regs, reg_count_field, reg_save_index, reg_padding) = - if gpr_type { (5, 0, 2, padding) } else { (4, 1, 16, 0) }; + let (max_regs, reg_count, reg_save_index, reg_padding) = + if gpr_type { (5, gpr, 2, padding) } else { (4, fpr, 16, 0) }; // Check whether the value was passed in a register or in memory. - let reg_count = bx.struct_gep( - va_list_ty, - va_list_addr, - va_list_layout.llvm_field_index(bx.cx, reg_count_field), - ); let reg_count_v = bx.load(bx.type_i64(), reg_count, Align::from_bytes(8).unwrap()); let use_regs = bx.icmp(IntPredicate::IntULT, reg_count_v, bx.const_u64(max_regs)); bx.cond_br(use_regs, in_reg, in_mem); @@ -209,9 +245,7 @@ fn emit_s390x_va_arg<'ll, 'tcx>( bx.switch_to_block(in_reg); // Work out the address of the value in the register save area. - let reg_ptr = - bx.struct_gep(va_list_ty, va_list_addr, va_list_layout.llvm_field_index(bx.cx, 3)); - let reg_ptr_v = bx.load(bx.type_ptr(), reg_ptr, bx.tcx().data_layout.pointer_align.abi); + let reg_ptr_v = bx.load(bx.type_ptr(), reg_save_area, dl.pointer_align.abi); let scaled_reg_count = bx.mul(reg_count_v, bx.const_u64(8)); let reg_off = bx.add(scaled_reg_count, bx.const_u64(reg_save_index * 8 + reg_padding)); let reg_addr = bx.gep(bx.type_i8(), reg_ptr_v, &[reg_off]); @@ -225,27 +259,23 @@ fn emit_s390x_va_arg<'ll, 'tcx>( bx.switch_to_block(in_mem); // Work out the address of the value in the argument overflow area. - let arg_ptr = - bx.struct_gep(va_list_ty, va_list_addr, va_list_layout.llvm_field_index(bx.cx, 2)); - let arg_ptr_v = bx.load(bx.type_ptr(), arg_ptr, bx.tcx().data_layout.pointer_align.abi); + let arg_ptr_v = + bx.load(bx.type_ptr(), overflow_arg_area, bx.tcx().data_layout.pointer_align.abi); let arg_off = bx.const_u64(padding); let mem_addr = bx.gep(bx.type_i8(), arg_ptr_v, &[arg_off]); // Update the argument overflow area pointer. let arg_size = bx.cx().const_u64(padded_size); let new_arg_ptr_v = bx.inbounds_gep(bx.type_i8(), arg_ptr_v, &[arg_size]); - bx.store(new_arg_ptr_v, arg_ptr, bx.tcx().data_layout.pointer_align.abi); + bx.store(new_arg_ptr_v, overflow_arg_area, dl.pointer_align.abi); bx.br(end); // Return the appropriate result. bx.switch_to_block(end); let val_addr = bx.phi(bx.type_ptr(), &[reg_addr, mem_addr], &[in_reg, in_mem]); let val_type = layout.llvm_type(bx); - let val_addr = if indirect { - bx.load(bx.cx.type_ptr(), val_addr, bx.tcx().data_layout.pointer_align.abi) - } else { - val_addr - }; + let val_addr = + if indirect { bx.load(bx.cx.type_ptr(), val_addr, dl.pointer_align.abi) } else { val_addr }; bx.load(val_type, val_addr, layout.align.abi) } diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index 86d3d1260c307..9689354d9b76f 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -190,7 +190,6 @@ pub trait BuilderMethods<'a, 'tcx>: ptr: Self::Value, indices: &[Self::Value], ) -> Self::Value; - fn struct_gep(&mut self, ty: Self::Type, ptr: Self::Value, idx: u64) -> Self::Value; fn trunc(&mut self, val: Self::Value, dest_ty: Self::Type) -> Self::Value; fn sext(&mut self, val: Self::Value, dest_ty: Self::Type) -> Self::Value; From 4724cd4dc4a4776ddedf3612180acfe172c16997 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Sat, 24 Feb 2024 02:01:41 -0500 Subject: [PATCH 3/5] introduce and use ptradd/inbounds_ptradd instead of gep --- compiler/rustc_codegen_gcc/src/builder.rs | 6 +-- compiler/rustc_codegen_llvm/src/builder.rs | 6 +-- compiler/rustc_codegen_llvm/src/va_arg.rs | 38 ++++++++----------- compiler/rustc_codegen_ssa/src/mir/operand.rs | 5 +-- compiler/rustc_codegen_ssa/src/mir/place.rs | 4 +- .../rustc_codegen_ssa/src/traits/builder.rs | 6 +++ 6 files changed, 27 insertions(+), 38 deletions(-) diff --git a/compiler/rustc_codegen_gcc/src/builder.rs b/compiler/rustc_codegen_gcc/src/builder.rs index bae0cc3655df6..71a0a4c2e96f2 100644 --- a/compiler/rustc_codegen_gcc/src/builder.rs +++ b/compiler/rustc_codegen_gcc/src/builder.rs @@ -839,11 +839,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> { let llptr = if i == 0 { place.llval } else { - self.inbounds_gep( - self.type_i8(), - place.llval, - &[self.const_usize(b_offset.bytes())], - ) + self.inbounds_ptradd(place.llval, self.const_usize(b_offset.bytes())) }; let llty = place.layout.scalar_pair_element_gcc_type(self, i); let load = self.load(llty, llptr, align); diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index 146fa591926c0..8ac7ae00b6d95 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -603,11 +603,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { let llptr = if i == 0 { place.llval } else { - self.inbounds_gep( - self.type_i8(), - place.llval, - &[self.const_usize(b_offset.bytes())], - ) + self.inbounds_ptradd(place.llval, self.const_usize(b_offset.bytes())) }; let llty = place.layout.scalar_pair_element_llvm_type(self, i, false); let load = self.load(llty, llptr, align); diff --git a/compiler/rustc_codegen_llvm/src/va_arg.rs b/compiler/rustc_codegen_llvm/src/va_arg.rs index 2d6cd51aead75..b406a04af7474 100644 --- a/compiler/rustc_codegen_llvm/src/va_arg.rs +++ b/compiler/rustc_codegen_llvm/src/va_arg.rs @@ -44,12 +44,12 @@ fn emit_direct_ptr_va_arg<'ll, 'tcx>( let aligned_size = size.align_to(slot_size).bytes() as i32; let full_direct_size = bx.cx().const_i32(aligned_size); - let next = bx.inbounds_gep(bx.type_i8(), addr, &[full_direct_size]); + let next = bx.inbounds_ptradd(addr, full_direct_size); bx.store(next, va_list_addr, bx.tcx().data_layout.pointer_align.abi); if size.bytes() < slot_size.bytes() && bx.tcx().sess.target.endian == Endian::Big { let adjusted_size = bx.cx().const_i32((slot_size.bytes() - size.bytes()) as i32); - let adjusted = bx.inbounds_gep(bx.type_i8(), addr, &[adjusted_size]); + let adjusted = bx.inbounds_ptradd(addr, adjusted_size); (adjusted, addr_align) } else { (addr, addr_align) @@ -109,14 +109,10 @@ fn emit_aapcs_va_arg<'ll, 'tcx>( // Table 3, Mapping of C & C++ built-in data types let ptr_offset = 8; let i32_offset = 4; - let gr_top = bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(ptr_offset)]); - let vr_top = bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(2 * ptr_offset)]); - let gr_offs = bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(3 * ptr_offset)]); - let vr_offs = bx.inbounds_gep( - bx.type_i8(), - va_list_addr, - &[bx.cx.const_usize(3 * ptr_offset + i32_offset)], - ); + let gr_top = bx.inbounds_ptradd(va_list_addr, bx.cx.const_usize(ptr_offset)); + let vr_top = bx.inbounds_ptradd(va_list_addr, bx.cx.const_usize(2 * ptr_offset)); + let gr_offs = bx.inbounds_ptradd(va_list_addr, bx.cx.const_usize(3 * ptr_offset)); + let vr_offs = bx.inbounds_ptradd(va_list_addr, bx.cx.const_usize(3 * ptr_offset + i32_offset)); let layout = bx.cx.layout_of(target_ty); @@ -164,11 +160,11 @@ fn emit_aapcs_va_arg<'ll, 'tcx>( let top = bx.load(top_type, reg_top, dl.pointer_align.abi); // reg_value = *(@top + reg_off_v); - let mut reg_addr = bx.gep(bx.type_i8(), top, &[reg_off_v]); + let mut reg_addr = bx.ptradd(top, reg_off_v); if bx.tcx().sess.target.endian == Endian::Big && layout.size.bytes() != slot_size { // On big-endian systems the value is right-aligned in its slot. let offset = bx.const_i32((slot_size - layout.size.bytes()) as i32); - reg_addr = bx.gep(bx.type_i8(), reg_addr, &[offset]); + reg_addr = bx.ptradd(reg_addr, offset); } let reg_type = layout.llvm_type(bx); let reg_value = bx.load(reg_type, reg_addr, layout.align.abi); @@ -210,14 +206,10 @@ fn emit_s390x_va_arg<'ll, 'tcx>( let i64_offset = 8; let ptr_offset = 8; let gpr = va_list_addr; - let fpr = bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(i64_offset)]); - let overflow_arg_area = - bx.inbounds_gep(bx.type_i8(), va_list_addr, &[bx.cx.const_usize(2 * i64_offset)]); - let reg_save_area = bx.inbounds_gep( - bx.type_i8(), - va_list_addr, - &[bx.cx.const_usize(2 * i64_offset + ptr_offset)], - ); + let fpr = bx.inbounds_ptradd(va_list_addr, bx.cx.const_usize(i64_offset)); + let overflow_arg_area = bx.inbounds_ptradd(va_list_addr, bx.cx.const_usize(2 * i64_offset)); + let reg_save_area = + bx.inbounds_ptradd(va_list_addr, bx.cx.const_usize(2 * i64_offset + ptr_offset)); let layout = bx.cx.layout_of(target_ty); @@ -248,7 +240,7 @@ fn emit_s390x_va_arg<'ll, 'tcx>( let reg_ptr_v = bx.load(bx.type_ptr(), reg_save_area, dl.pointer_align.abi); let scaled_reg_count = bx.mul(reg_count_v, bx.const_u64(8)); let reg_off = bx.add(scaled_reg_count, bx.const_u64(reg_save_index * 8 + reg_padding)); - let reg_addr = bx.gep(bx.type_i8(), reg_ptr_v, &[reg_off]); + let reg_addr = bx.ptradd(reg_ptr_v, reg_off); // Update the register count. let new_reg_count_v = bx.add(reg_count_v, bx.const_u64(1)); @@ -262,11 +254,11 @@ fn emit_s390x_va_arg<'ll, 'tcx>( let arg_ptr_v = bx.load(bx.type_ptr(), overflow_arg_area, bx.tcx().data_layout.pointer_align.abi); let arg_off = bx.const_u64(padding); - let mem_addr = bx.gep(bx.type_i8(), arg_ptr_v, &[arg_off]); + let mem_addr = bx.ptradd(arg_ptr_v, arg_off); // Update the argument overflow area pointer. let arg_size = bx.cx().const_u64(padded_size); - let new_arg_ptr_v = bx.inbounds_gep(bx.type_i8(), arg_ptr_v, &[arg_size]); + let new_arg_ptr_v = bx.inbounds_ptradd(arg_ptr_v, arg_size); bx.store(new_arg_ptr_v, overflow_arg_area, dl.pointer_align.abi); bx.br(end); diff --git a/compiler/rustc_codegen_ssa/src/mir/operand.rs b/compiler/rustc_codegen_ssa/src/mir/operand.rs index 6f6f010422f8b..94eb37e78e07d 100644 --- a/compiler/rustc_codegen_ssa/src/mir/operand.rs +++ b/compiler/rustc_codegen_ssa/src/mir/operand.rs @@ -437,8 +437,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue { let align = dest.align; bx.store_with_flags(val, dest.llval, align, flags); - let llptr = - bx.inbounds_gep(bx.type_i8(), dest.llval, &[bx.const_usize(b_offset.bytes())]); + let llptr = bx.inbounds_ptradd(dest.llval, bx.const_usize(b_offset.bytes())); let val = bx.from_immediate(b); let align = dest.align.restrict_for_offset(b_offset); bx.store_with_flags(val, llptr, align, flags); @@ -476,7 +475,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandValue { let address = bx.ptrtoint(alloca, bx.type_isize()); let neg_address = bx.neg(address); let offset = bx.and(neg_address, align_minus_1); - let dst = bx.inbounds_gep(bx.type_i8(), alloca, &[offset]); + let dst = bx.inbounds_ptradd(alloca, offset); bx.memcpy(dst, min_align, llptr, min_align, size, MemFlags::empty()); // Store the allocated region and the extra to the indirect place. diff --git a/compiler/rustc_codegen_ssa/src/mir/place.rs b/compiler/rustc_codegen_ssa/src/mir/place.rs index 0ce890dca3204..09ff64b98c2b8 100644 --- a/compiler/rustc_codegen_ssa/src/mir/place.rs +++ b/compiler/rustc_codegen_ssa/src/mir/place.rs @@ -105,7 +105,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { let llval = if offset.bytes() == 0 { self.llval } else { - bx.inbounds_gep(bx.type_i8(), self.llval, &[bx.const_usize(offset.bytes())]) + bx.inbounds_ptradd(self.llval, bx.const_usize(offset.bytes())) }; PlaceRef { llval, @@ -164,7 +164,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> { debug!("struct_field_ptr: DST field offset: {:?}", offset); // Adjust pointer. - let ptr = bx.gep(bx.cx().type_i8(), self.llval, &[offset]); + let ptr = bx.ptradd(self.llval, offset); PlaceRef { llval: ptr, llextra: self.llextra, layout: field, align: effective_field_align } } diff --git a/compiler/rustc_codegen_ssa/src/traits/builder.rs b/compiler/rustc_codegen_ssa/src/traits/builder.rs index 9689354d9b76f..36f37e3791bc5 100644 --- a/compiler/rustc_codegen_ssa/src/traits/builder.rs +++ b/compiler/rustc_codegen_ssa/src/traits/builder.rs @@ -190,6 +190,12 @@ pub trait BuilderMethods<'a, 'tcx>: ptr: Self::Value, indices: &[Self::Value], ) -> Self::Value; + fn ptradd(&mut self, ptr: Self::Value, offset: Self::Value) -> Self::Value { + self.gep(self.cx().type_i8(), ptr, &[offset]) + } + fn inbounds_ptradd(&mut self, ptr: Self::Value, offset: Self::Value) -> Self::Value { + self.inbounds_gep(self.cx().type_i8(), ptr, &[offset]) + } fn trunc(&mut self, val: Self::Value, dest_ty: Self::Type) -> Self::Value; fn sext(&mut self, val: Self::Value, dest_ty: Self::Type) -> Self::Value; From c1017d48281c0fad0a93e36315db357d3a8b5f6a Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Tue, 27 Feb 2024 23:00:54 -0500 Subject: [PATCH 4/5] use non-inbounds GEP for ZSTs, add fixmes --- compiler/rustc_codegen_ssa/src/mir/place.rs | 5 +++++ tests/codegen/zst-offset.rs | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/mir/place.rs b/compiler/rustc_codegen_ssa/src/mir/place.rs index 09ff64b98c2b8..725d3bf4431e4 100644 --- a/compiler/rustc_codegen_ssa/src/mir/place.rs +++ b/compiler/rustc_codegen_ssa/src/mir/place.rs @@ -104,6 +104,10 @@ 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())) }; @@ -164,6 +168,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); PlaceRef { llval: ptr, llextra: self.llextra, layout: field, align: effective_field_align } diff --git a/tests/codegen/zst-offset.rs b/tests/codegen/zst-offset.rs index b623d492d9d69..65d9cf39c4c61 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 inbounds i8, {{.+}}, [[USIZE]] 8 +// CHECK: getelementptr 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 inbounds i8, {{.+}}, [[USIZE]] 12 +// CHECK: getelementptr 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 inbounds i8, {{.+}}, [[USIZE]] 32 +// CHECK: getelementptr i8, {{.+}}, [[USIZE]] 32 let x = &s.1; witness(&x); // keep variable in an alloca } From 401651015d837ebfe55928692a84de37ec8a3a4b Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Tue, 27 Feb 2024 23:14:36 -0500 Subject: [PATCH 5/5] test merging of multiple match branches that access fields of the same offset --- .../issue-121719-common-field-offset.rs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 tests/codegen/issues/issue-121719-common-field-offset.rs diff --git a/tests/codegen/issues/issue-121719-common-field-offset.rs b/tests/codegen/issues/issue-121719-common-field-offset.rs new file mode 100644 index 0000000000000..11a8aa8dcd144 --- /dev/null +++ b/tests/codegen/issues/issue-121719-common-field-offset.rs @@ -0,0 +1,44 @@ +//! This test checks that match branches which all access a field +//! at the same offset are merged together. +//! +//@ compile-flags: -O +#![crate_type = "lib"] + +#[repr(C)] +pub struct A { + x: f64, + y: u64, +} +#[repr(C)] +pub struct B { + x: f64, + y: u32, +} +#[repr(C)] +pub struct C { + x: f64, + y: u16, +} +#[repr(C)] +pub struct D { + x: f64, + y: u8, +} + +pub enum E { + A(A), + B(B), + C(C), + D(D), +} + +// CHECK-LABEL: @match_on_e +#[no_mangle] +pub fn match_on_e(e: &E) -> &f64 { + // CHECK: start: + // CHECK-NEXT: getelementptr + // CHECK-NEXT: ret + match e { + E::A(A { x, .. }) | E::B(B { x, .. }) | E::C(C { x, .. }) | E::D(D { x, .. }) => x, + } +}