From 2100b10c4c61ac7c0fe67ae4c9a390eaf3708ae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Steinbrink?= Date: Fri, 9 Jan 2015 17:40:13 +0100 Subject: [PATCH 1/3] Treat `struct(T)` the same as `struct S { x: T }` WRT being immediate args Currently we pass a `struct S(u64)` as an immediate value on i686, but a `struct S { x: u64 }` is passed indirectly. This seems pretty wrong, as they both have the same underlying LLVM type `{ i64 }`, no sense in treating them differently. --- src/librustc_trans/trans/common.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/librustc_trans/trans/common.rs b/src/librustc_trans/trans/common.rs index 3afd33d324d..fb58d935922 100644 --- a/src/librustc_trans/trans/common.rs +++ b/src/librustc_trans/trans/common.rs @@ -222,10 +222,7 @@ fn type_is_newtype_immediate<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, match ty.sty { ty::ty_struct(def_id, substs) => { let fields = ty::struct_fields(ccx.tcx(), def_id, substs); - fields.len() == 1 && - fields[0].name == - token::special_idents::unnamed_field.name && - type_is_immediate(ccx, fields[0].mt.ty) + fields.len() == 1 && type_is_immediate(ccx, fields[0].mt.ty) } _ => false } From 6ef08406dccdeeb828a6cfc7658ac149900e73ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Steinbrink?= Date: Thu, 8 Jan 2015 16:28:07 +0100 Subject: [PATCH 2/3] Avoid loads/stores of first class aggregates Currently, small aggregates are passed to functions as immediate values as is. This has two consequences. One is that aggregates are passed component-wise by LLVM, so e.g. a struct containing four u8 values (e.g. an RGBA struct) will be passed as four individual values. The other is that LLVM isn't very good at optimizing loads/stores of first class attributes. What clang does is converting the aggregate to an appropriately sized integer type (e.g. i32 for the four u8 values), and using that for the function argument. This allows LLVM to create code that is a lot better. Fixes #20450 #20149 #16506 #13927 --- src/librustc_trans/trans/base.rs | 12 +++++++++++- src/librustc_trans/trans/foreign.rs | 11 +++++++++-- src/librustc_trans/trans/glue.rs | 2 +- src/librustc_trans/trans/intrinsic.rs | 13 +++++++++---- src/librustc_trans/trans/type_.rs | 12 ++++++++++++ src/librustc_trans/trans/type_of.rs | 15 +++++++++++++++ 6 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 88ce36a710a..ea98d6bb74e 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1054,6 +1054,11 @@ pub fn load_ty<'blk, 'tcx>(cx: Block<'blk, 'tcx>, C_undef(type_of::type_of(cx.ccx(), t)) } else if ty::type_is_bool(t) { Trunc(cx, LoadRangeAssert(cx, ptr, 0, 2, llvm::False), Type::i1(cx.ccx())) + } else if type_is_immediate(cx.ccx(), t) && type_of::type_of(cx.ccx(), t).is_aggregate() { + // We want to pass small aggregates as immediate values, but using an aggregate LLVM type + // for this leads to bad optimizations, so its arg type is an appropriately sized integer + // and we have to convert it + Load(cx, BitCast(cx, ptr, type_of::arg_type_of(cx.ccx(), t).ptr_to())) } else if ty::type_is_char(t) { // a char is a Unicode codepoint, and so takes values from 0 // to 0x10FFFF inclusive only. @@ -1065,9 +1070,14 @@ pub fn load_ty<'blk, 'tcx>(cx: Block<'blk, 'tcx>, /// Helper for storing values in memory. Does the necessary conversion if the in-memory type /// differs from the type used for SSA values. -pub fn store_ty(cx: Block, v: ValueRef, dst: ValueRef, t: Ty) { +pub fn store_ty<'blk, 'tcx>(cx: Block<'blk, 'tcx>, v: ValueRef, dst: ValueRef, t: Ty<'tcx>) { if ty::type_is_bool(t) { Store(cx, ZExt(cx, v, Type::i8(cx.ccx())), dst); + } else if type_is_immediate(cx.ccx(), t) && type_of::type_of(cx.ccx(), t).is_aggregate() { + // We want to pass small aggregates as immediate values, but using an aggregate LLVM type + // for this leads to bad optimizations, so its arg type is an appropriately sized integer + // and we have to convert it + Store(cx, v, BitCast(cx, dst, type_of::arg_type_of(cx.ccx(), t).ptr_to())); } else { Store(cx, v, dst); }; diff --git a/src/librustc_trans/trans/foreign.rs b/src/librustc_trans/trans/foreign.rs index 3dfb36c854b..fb61bab6ade 100644 --- a/src/librustc_trans/trans/foreign.rs +++ b/src/librustc_trans/trans/foreign.rs @@ -736,6 +736,13 @@ pub fn trans_rust_fn_with_foreign_abi<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, if ty::type_is_bool(rust_ty) { let tmp = builder.load_range_assert(llforeign_arg, 0, 2, llvm::False); builder.trunc(tmp, Type::i1(ccx)) + } else if type_of::type_of(ccx, rust_ty).is_aggregate() { + // We want to pass small aggregates as immediate values, but using an aggregate + // LLVM type for this leads to bad optimizations, so its arg type is an + // appropriately sized integer and we have to convert it + let tmp = builder.bitcast(llforeign_arg, + type_of::arg_type_of(ccx, rust_ty).ptr_to()); + builder.load(tmp) } else { builder.load(llforeign_arg) } @@ -834,10 +841,10 @@ fn foreign_signature<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, fn_sig: &ty::FnSig<'tcx>, arg_tys: &[Ty<'tcx>]) -> LlvmSignature { - let llarg_tys = arg_tys.iter().map(|&arg| arg_type_of(ccx, arg)).collect(); + let llarg_tys = arg_tys.iter().map(|&arg| foreign_arg_type_of(ccx, arg)).collect(); let (llret_ty, ret_def) = match fn_sig.output { ty::FnConverging(ret_ty) => - (type_of::arg_type_of(ccx, ret_ty), !return_type_is_void(ccx, ret_ty)), + (type_of::foreign_arg_type_of(ccx, ret_ty), !return_type_is_void(ccx, ret_ty)), ty::FnDiverging => (Type::nil(ccx), false) }; diff --git a/src/librustc_trans/trans/glue.rs b/src/librustc_trans/trans/glue.rs index 2219cd59263..a79bb6ca164 100644 --- a/src/librustc_trans/trans/glue.rs +++ b/src/librustc_trans/trans/glue.rs @@ -138,7 +138,7 @@ pub fn drop_ty_immediate<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, -> Block<'blk, 'tcx> { let _icx = push_ctxt("drop_ty_immediate"); let vp = alloca(bcx, type_of(bcx.ccx(), t), ""); - Store(bcx, v, vp); + store_ty(bcx, v, vp, t); drop_ty(bcx, vp, t, source_location) } diff --git a/src/librustc_trans/trans/intrinsic.rs b/src/librustc_trans/trans/intrinsic.rs index b22c7f763f0..91c7409182d 100644 --- a/src/librustc_trans/trans/intrinsic.rs +++ b/src/librustc_trans/trans/intrinsic.rs @@ -357,11 +357,11 @@ pub fn trans_intrinsic_call<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>, &ccx.link_meta().crate_hash); // NB: This needs to be kept in lockstep with the TypeId struct in // the intrinsic module - C_named_struct(llret_ty, &[C_u64(ccx, hash)]) + C_u64(ccx, hash) } (_, "init") => { let tp_ty = *substs.types.get(FnSpace, 0); - let lltp_ty = type_of::type_of(ccx, tp_ty); + let lltp_ty = type_of::arg_type_of(ccx, tp_ty); if return_type_is_void(ccx, tp_ty) { C_nil(ccx) } else { @@ -686,6 +686,11 @@ fn with_overflow_intrinsic<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, name: &'static st let ret = C_undef(type_of::type_of(bcx.ccx(), t)); let ret = InsertValue(bcx, ret, result, 0); let ret = InsertValue(bcx, ret, overflow, 1); - - ret + if type_is_immediate(bcx.ccx(), t) { + let tmp = alloc_ty(bcx, t, "tmp"); + Store(bcx, ret, tmp); + load_ty(bcx, tmp, t) + } else { + ret + } } diff --git a/src/librustc_trans/trans/type_.rs b/src/librustc_trans/trans/type_.rs index 71a7789eb39..9cae142c03a 100644 --- a/src/librustc_trans/trans/type_.rs +++ b/src/librustc_trans/trans/type_.rs @@ -82,6 +82,11 @@ impl Type { ty!(llvm::LLVMInt64TypeInContext(ccx.llcx())) } + // Creates an integer type with the given number of bits, e.g. i24 + pub fn ix(ccx: &CrateContext, num_bits: u64) -> Type { + ty!(llvm::LLVMIntTypeInContext(ccx.llcx(), num_bits as c_uint)) + } + pub fn f32(ccx: &CrateContext) -> Type { ty!(llvm::LLVMFloatTypeInContext(ccx.llcx())) } @@ -260,6 +265,13 @@ impl Type { ty!(llvm::LLVMPointerType(self.to_ref(), 0)) } + pub fn is_aggregate(&self) -> bool { + match self.kind() { + TypeKind::Struct | TypeKind::Array => true, + _ => false + } + } + pub fn is_packed(&self) -> bool { unsafe { llvm::LLVMIsPackedStruct(self.to_ref()) == True diff --git a/src/librustc_trans/trans/type_of.rs b/src/librustc_trans/trans/type_of.rs index 99330797422..76e0e0d0545 100644 --- a/src/librustc_trans/trans/type_of.rs +++ b/src/librustc_trans/trans/type_of.rs @@ -243,9 +243,24 @@ pub fn sizing_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>) -> Typ llsizingty } +pub fn foreign_arg_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>) -> Type { + if ty::type_is_bool(t) { + Type::i1(cx) + } else { + type_of(cx, t) + } +} + pub fn arg_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>) -> Type { if ty::type_is_bool(t) { Type::i1(cx) + } else if type_is_immediate(cx, t) && type_of(cx, t).is_aggregate() { + // We want to pass small aggregates as immediate values, but using an aggregate LLVM type + // for this leads to bad optimizations, so its arg type is an appropriately sized integer + match machine::llsize_of_alloc(cx, sizing_type_of(cx, t)) { + 0 => type_of(cx, t), + n => Type::ix(cx, n * 8), + } } else { type_of(cx, t) } From 6f1b06eb6546ab4808614dfe7807277419b485aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Steinbrink?= Date: Thu, 8 Jan 2015 16:34:09 +0100 Subject: [PATCH 3/3] Pass small fixed-size arrays as immediates Currently even small fixed-size arrays are passed as indirect parameters, which seems to be just an oversight. Let's handle them the same as structs of the same size, passing them as immediate values. --- src/librustc_trans/trans/common.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_trans/trans/common.rs b/src/librustc_trans/trans/common.rs index fb58d935922..f59e70d099a 100644 --- a/src/librustc_trans/trans/common.rs +++ b/src/librustc_trans/trans/common.rs @@ -244,7 +244,7 @@ pub fn type_is_immediate<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, ty: Ty<'tcx>) - return false; } match ty.sty { - ty::ty_struct(..) | ty::ty_enum(..) | ty::ty_tup(..) | + ty::ty_struct(..) | ty::ty_enum(..) | ty::ty_tup(..) | ty::ty_vec(_, Some(_)) | ty::ty_unboxed_closure(..) => { let llty = sizing_type_of(ccx, ty); llsize_of_alloc(ccx, llty) <= llsize_of_alloc(ccx, ccx.int_type())