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

codegen_llvm/llvm_type: avoid matching on the Rust type #115240

Merged
merged 4 commits into from
Aug 28, 2023
Merged

codegen_llvm/llvm_type: avoid matching on the Rust type #115240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fde82f
codegen_llvm/llvm_type: avoid matching on the Rust type
RalfJung Aug 26, 2023
dc70fb6
also avoid matching on the type in scalar_pair_element_llvm_type
RalfJung Aug 28, 2023
99d76a4
carry out the same changes in the gcc backend
RalfJung Aug 28, 2023
9b9cb51
remove an unused argument
RalfJung Aug 28, 2023
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
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_gcc/src/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ impl<'gcc, 'tcx> FnAbiGccExt<'gcc, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
PassMode::Ignore => continue,
PassMode::Direct(_) => arg.layout.immediate_gcc_type(cx),
PassMode::Pair(..) => {
argument_tys.push(arg.layout.scalar_pair_element_gcc_type(cx, 0, true));
argument_tys.push(arg.layout.scalar_pair_element_gcc_type(cx, 1, true));
argument_tys.push(arg.layout.scalar_pair_element_gcc_type(cx, 0));
argument_tys.push(arg.layout.scalar_pair_element_gcc_type(cx, 1));
continue;
}
PassMode::Indirect { extra_attrs: Some(_), .. } => {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_gcc/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> {

let mut load = |i, scalar: &abi::Scalar, align| {
let llptr = self.struct_gep(pair_type, place.llval, i as u64);
let llty = place.layout.scalar_pair_element_gcc_type(self, i, false);
let llty = place.layout.scalar_pair_element_gcc_type(self, i);
let load = self.load(llty, llptr, align);
scalar_load_metadata(self, load, scalar);
if scalar.is_bool() { self.trunc(load, self.type_i1()) } else { load }
Expand Down
48 changes: 14 additions & 34 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 @@ -4,7 +4,7 @@ use gccjit::{Struct, Type};
use crate::rustc_codegen_ssa::traits::{BaseTypeMethods, DerivedTypeMethods, LayoutTypeMethods};
use rustc_middle::bug;
use rustc_middle::ty::{self, Ty, TypeVisitableExt};
use rustc_middle::ty::layout::{FnAbiOf, LayoutOf, TyAndLayout};
use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_target::abi::{self, Abi, Align, F32, F64, FieldsShape, Int, Integer, Pointer, PointeeInfo, Size, TyAbiInterface, Variants};
use rustc_target::abi::call::{CastTarget, FnAbi, Reg};
Expand Down Expand Up @@ -74,8 +74,8 @@ fn uncached_gcc_type<'gcc, 'tcx>(cx: &CodegenCx<'gcc, 'tcx>, layout: TyAndLayout
Abi::ScalarPair(..) => {
return cx.type_struct(
&[
layout.scalar_pair_element_gcc_type(cx, 0, false),
layout.scalar_pair_element_gcc_type(cx, 1, false),
layout.scalar_pair_element_gcc_type(cx, 0),
layout.scalar_pair_element_gcc_type(cx, 1),
],
false,
);
Expand Down Expand Up @@ -150,7 +150,7 @@ pub trait LayoutGccExt<'tcx> {
fn gcc_type<'gcc>(&self, cx: &CodegenCx<'gcc, 'tcx>) -> Type<'gcc>;
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, immediate: bool) -> 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 @@ -182,23 +182,16 @@ impl<'tcx> LayoutGccExt<'tcx> for TyAndLayout<'tcx> {
/// of that field's type - this is useful for taking the address of
/// that field and ensuring the struct has the right alignment.
fn gcc_type<'gcc>(&self, cx: &CodegenCx<'gcc, 'tcx>) -> Type<'gcc> {
// This must produce the same result for `repr(transparent)` wrappers as for the inner type!
// In other words, this should generally not look at the type at all, but only at the
// layout.
if let Abi::Scalar(ref scalar) = self.abi {
// Use a different cache for scalars because pointers to DSTs
// can be either fat or thin (data pointers of fat pointers).
if let Some(&ty) = cx.scalar_types.borrow().get(&self.ty) {
return ty;
}
let ty =
match *self.ty.kind() {
ty::Ref(_, ty, _) | ty::RawPtr(ty::TypeAndMut { ty, .. }) => {
cx.type_ptr_to(cx.layout_of(ty).gcc_type(cx))
}
ty::Adt(def, _) if def.is_box() => {
cx.type_ptr_to(cx.layout_of(self.ty.boxed_ty()).gcc_type(cx))
}
ty::FnPtr(sig) => cx.fn_ptr_backend_type(&cx.fn_abi_of_fn_ptr(sig, ty::List::empty())),
_ => self.scalar_gcc_type_at(cx, scalar, Size::ZERO),
};
let ty = self.scalar_gcc_type_at(cx, scalar, Size::ZERO);
cx.scalar_types.borrow_mut().insert(self.ty, ty);
return ty;
}
Expand Down Expand Up @@ -272,23 +265,10 @@ impl<'tcx> LayoutGccExt<'tcx> for TyAndLayout<'tcx> {
}
}

fn scalar_pair_element_gcc_type<'gcc>(&self, cx: &CodegenCx<'gcc, 'tcx>, index: usize, immediate: bool) -> Type<'gcc> {
// TODO(antoyo): remove llvm hack:
// HACK(eddyb) special-case fat pointers until LLVM removes
// pointee types, to avoid bitcasting every `OperandRef::deref`.
match self.ty.kind() {
ty::Ref(..) | ty::RawPtr(_) => {
return self.field(cx, index).gcc_type(cx);
}
// only wide pointer boxes are handled as pointers
// thin pointer boxes with scalar allocators are handled by the general logic below
ty::Adt(def, args) if def.is_box() && cx.layout_of(args.type_at(1)).is_zst() => {
let ptr_ty = Ty::new_mut_ptr(cx.tcx,self.ty.boxed_ty());
return cx.layout_of(ptr_ty).scalar_pair_element_gcc_type(cx, index, immediate);
}
_ => {}
}

fn scalar_pair_element_gcc_type<'gcc>(&self, cx: &CodegenCx<'gcc, 'tcx>, index: usize) -> Type<'gcc> {
// This must produce the same result for `repr(transparent)` wrappers as for the inner type!
// In other words, this should generally not look at the type at all, but only at the
// layout.
let (a, b) = match self.abi {
Abi::ScalarPair(ref a, ref b) => (a, b),
_ => bug!("TyAndLayout::scalar_pair_element_llty({:?}): not applicable", self),
Expand Down Expand Up @@ -367,8 +347,8 @@ impl<'gcc, 'tcx> LayoutTypeMethods<'tcx> for CodegenCx<'gcc, 'tcx> {
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, immediate)
fn scalar_pair_element_backend_type(&self, layout: TyAndLayout<'tcx>, index: usize, _immediate: bool) -> Type<'gcc> {
layout.scalar_pair_element_gcc_type(self, index)
}

fn cast_backend_type(&self, ty: &CastTarget) -> Type<'gcc> {
Expand Down
38 changes: 8 additions & 30 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 @@ -3,7 +3,7 @@ use crate::context::TypeLowering;
use crate::type_::Type;
use rustc_codegen_ssa::traits::*;
use rustc_middle::bug;
use rustc_middle::ty::layout::{FnAbiOf, LayoutOf, TyAndLayout};
use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
use rustc_middle::ty::print::{with_no_trimmed_paths, with_no_visible_paths};
use rustc_middle::ty::{self, Ty, TypeVisitableExt};
use rustc_target::abi::HasDataLayout;
Expand Down Expand Up @@ -215,20 +215,16 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyAndLayout<'tcx> {
/// of that field's type - this is useful for taking the address of
/// that field and ensuring the struct has the right alignment.
fn llvm_type<'a>(&self, cx: &CodegenCx<'a, 'tcx>) -> &'a Type {
// This must produce the same result for `repr(transparent)` wrappers as for the inner type!
// In other words, this should generally not look at the type at all, but only at the
// layout.
Comment on lines +218 to +220
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for seeing this so late, but, if not made clear so far by anyone else:
the special cases you removed are simultaneously sound and obsolete.

They're sound because they only affect pointer types' pointee types (which doesn't impact ABI on it own), and they're obsolete thanks to LLVM opaque pointers (making the removal welcome!).

You can kinda tell they were working fine because e.g. Option<&String> would look like i64*, whereas the inner &String would actually be {i8*, i64, i64}* - worst case, it would pessimize some badly written LLVM optimizations, but all of those concerns should be gone now.

In fact, some of this code can very likely be simplified!

scalar_pair_element_llvm_type only exists in the form it does because it needed to special-case wide pointers, but now each component of the scalar pair can be independently converted to LLVM.

It might even make sense to remove caches like scalar_lltypes and instead have something much simpler keyed by Primitive - the existing scalar_llvm_type_at might be good enough, even, and I would argue the cache only existed because of the pointer cases, and can now be fully removed.

Copy link
Member Author

@RalfJung RalfJung Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the special cases you removed are simultaneously sound and obsolete.

Yes that was my conclusion, too. That still makes this PR a good cleanup, no? :)

But looking at these special cases led me to ones that aren't sound (see the recent ABI issues I opened), so the cleanup of obsolete-but-sound type matching was still worth it.

I'm sure there's further simplification possible.

if let Abi::Scalar(scalar) = self.abi {
// Use a different cache for scalars because pointers to DSTs
// can be either fat or thin (data pointers of fat pointers).
if let Some(&llty) = cx.scalar_lltypes.borrow().get(&self.ty) {
return llty;
}
let llty = match *self.ty.kind() {
ty::Ref(..) | ty::RawPtr(_) => cx.type_ptr(),
ty::Adt(def, _) if def.is_box() => cx.type_ptr(),
ty::FnPtr(sig) => {
cx.fn_ptr_backend_type(cx.fn_abi_of_fn_ptr(sig, ty::List::empty()))
}
_ => self.scalar_llvm_type_at(cx, scalar),
};
let llty = self.scalar_llvm_type_at(cx, scalar);
cx.scalar_lltypes.borrow_mut().insert(self.ty, llty);
return llty;
}
Expand Down Expand Up @@ -303,27 +299,9 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyAndLayout<'tcx> {
index: usize,
immediate: bool,
) -> &'a Type {
// HACK(eddyb) special-case fat pointers until LLVM removes
// pointee types, to avoid bitcasting every `OperandRef::deref`.
match *self.ty.kind() {
ty::Ref(..) | ty::RawPtr(_) => {
return self.field(cx, index).llvm_type(cx);
}
// only wide pointer boxes are handled as pointers
// thin pointer boxes with scalar allocators are handled by the general logic below
ty::Adt(def, args) if def.is_box() && cx.layout_of(args.type_at(1)).is_zst() => {
let ptr_ty = Ty::new_mut_ptr(cx.tcx, self.ty.boxed_ty());
return cx.layout_of(ptr_ty).scalar_pair_element_llvm_type(cx, index, immediate);
}
// `dyn* Trait` has the same ABI as `*mut dyn Trait`
ty::Dynamic(bounds, region, ty::DynStar) => {
let ptr_ty =
Ty::new_mut_ptr(cx.tcx, Ty::new_dynamic(cx.tcx, bounds, region, ty::Dyn));
return cx.layout_of(ptr_ty).scalar_pair_element_llvm_type(cx, index, immediate);
}
_ => {}
}

// This must produce the same result for `repr(transparent)` wrappers as for the inner type!
// In other words, this should generally not look at the type at all, but only at the
// layout.
let Abi::ScalarPair(a, b) = self.abi else {
bug!("TyAndLayout::scalar_pair_element_llty({:?}): not applicable", self);
};
Expand Down