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

Always generate GEP i8 / ptradd for struct offsets #121665

Merged
merged 5 commits into from
Mar 4, 2024
Merged

Always generate GEP i8 / ptradd for struct offsets #121665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
123015e
always use gep inbounds i8 (ptradd) for field offsets
erikdesjardins Feb 24, 2024
beed25b
remove struct_gep, use manual layout calculations for va_arg
erikdesjardins Feb 24, 2024
4724cd4
introduce and use ptradd/inbounds_ptradd instead of gep
erikdesjardins Feb 24, 2024
c1017d4
use non-inbounds GEP for ZSTs, add fixmes
erikdesjardins Feb 28, 2024
4016510
test merging of multiple match branches that access fields of the sam…
erikdesjardins Feb 28, 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: 5 additions & 29 deletions compiler/rustc_codegen_gcc/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,10 +834,13 @@ 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_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);
scalar_load_metadata(self, load, scalar);
Expand Down Expand Up @@ -971,33 +974,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.
Expand Down
23 changes: 0 additions & 23 deletions compiler/rustc_codegen_gcc/src/type_of.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<PointeeInfo>;
}

Expand Down Expand Up @@ -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<PointeeInfo> {
if let Some(&pointee) = cx.pointee_infos.borrow().get(&(self.ty, offset)) {
return pointee;
Expand Down Expand Up @@ -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)
}
Expand Down
11 changes: 1 addition & 10 deletions compiler/rustc_codegen_llvm/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -778,11 +774,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) }
Expand Down
7 changes: 0 additions & 7 deletions compiler/rustc_codegen_llvm/src/llvm/ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>(
Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_codegen_llvm/src/type_.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
37 changes: 0 additions & 37 deletions compiler/rustc_codegen_llvm/src/type_of.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
}

Expand Down Expand Up @@ -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());

Expand Down
100 changes: 61 additions & 39 deletions compiler/rustc_codegen_llvm/src/va_arg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -89,11 +89,31 @@ 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_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));
Comment on lines +97 to +115
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally I'd feel bad about this kind of manual layout computation, but this file is already packed full of hardcoded offsets and assumptions.

At some point we should re-evaluate whether we can use the LLVM va_arg directly.

To that point, I'm confused about the comment

The LLVM va_arg instruction is lacking in some instances, so we should only use it as a fallback.

...does Clang also implement this logic manually?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. LLVM types are not rich enough to encode all the necessary ABI requirements to make something like the va_arg instruction work correctly. It would probably be best to remove it entirely.


let layout = bx.cx.layout_of(target_ty);

let maybe_reg = bx.append_sibling_block("va_arg.maybe_reg");
Expand All @@ -104,16 +124,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
Expand Down Expand Up @@ -141,15 +157,14 @@ 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]);
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);
Expand All @@ -173,11 +188,29 @@ 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_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);

let in_reg = bx.append_sibling_block("va_arg.in_reg");
Expand All @@ -192,15 +225,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);
Expand All @@ -209,12 +237,10 @@ 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]);
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));
Expand All @@ -225,27 +251,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]);
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]);
bx.store(new_arg_ptr_v, arg_ptr, bx.tcx().data_layout.pointer_align.abi);
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);

// 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)
}

Expand Down
Loading
Loading