From dc262d9aa742adadb92558a588aeaa57267fdee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Steinbrink?= Date: Wed, 19 Jun 2013 20:28:42 +0200 Subject: [PATCH 1/2] Avoid unnecessary scratch datums for by-copy function arguments Currently, by-copy function arguments are always stored into a scratch datum, which serves two purposes. First, it is required to be able to have a temporary cleanup, in case that the call fails before the callee actually takes ownership of the value. Second, if the argument is to be passed by reference, the copy is required, so that the function doesn't get a reference to the original value. But in case that the datum does not need a drop glue call and it is passed by value, there's no need to perform the extra copy. --- src/librustc/middle/trans/callee.rs | 45 +++++++++++++++++------------ 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/src/librustc/middle/trans/callee.rs b/src/librustc/middle/trans/callee.rs index bfbe078c4f524..47e5f9bf35f80 100644 --- a/src/librustc/middle/trans/callee.rs +++ b/src/librustc/middle/trans/callee.rs @@ -826,24 +826,33 @@ pub fn trans_arg_expr(bcx: block, val = arg_datum.to_ref_llval(bcx); } ty::ByCopy => { - debug!("by copy arg with type %s, storing to scratch", - bcx.ty_to_str(arg_datum.ty)); - let scratch = scratch_datum(bcx, arg_datum.ty, false); - - arg_datum.store_to_datum(bcx, - arg_expr.id, - INIT, - scratch); - - // Technically, ownership of val passes to the callee. - // However, we must cleanup should we fail before the - // callee is actually invoked. - scratch.add_clean(bcx); - temp_cleanups.push(scratch.val); - - match arg_datum.appropriate_mode() { - ByValue => val = Load(bcx, scratch.val), - ByRef(_) => val = scratch.val, + if ty::type_needs_drop(bcx.tcx(), arg_datum.ty) || + arg_datum.appropriate_mode().is_by_ref() { + debug!("by copy arg with type %s, storing to scratch", + bcx.ty_to_str(arg_datum.ty)); + let scratch = scratch_datum(bcx, arg_datum.ty, false); + + arg_datum.store_to_datum(bcx, + arg_expr.id, + INIT, + scratch); + + // Technically, ownership of val passes to the callee. + // However, we must cleanup should we fail before the + // callee is actually invoked. + scratch.add_clean(bcx); + temp_cleanups.push(scratch.val); + + match scratch.appropriate_mode() { + ByValue => val = Load(bcx, scratch.val), + ByRef(_) => val = scratch.val, + } + } else { + debug!("by copy arg with type %s"); + match arg_datum.mode { + ByRef(_) => val = Load(bcx, arg_datum.val), + ByValue => val = arg_datum.val, + } } } } From 4fb2c09541a5cb7ea91785a9c972ee57ff110c62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Steinbrink?= Date: Thu, 20 Jun 2013 16:42:44 +0200 Subject: [PATCH 2/2] Avoid pointless allocas for "nil" return values By using "void" instead of "{}" as the LLVM type for nil, we can avoid the alloca/store/load sequence for the return value, resulting in less and simpler IR code. This reduces compile times by about 10%. --- src/librustc/middle/trans/base.rs | 9 +++++---- src/librustc/middle/trans/cabi.rs | 24 +++++++++++++----------- src/librustc/middle/trans/cabi_x86.rs | 5 +++++ src/librustc/middle/trans/closure.rs | 5 ++++- src/librustc/middle/trans/controlflow.rs | 13 ++++++++----- src/librustc/middle/trans/foreign.rs | 18 +++++++++++------- src/librustc/middle/trans/type_of.rs | 6 +++--- 7 files changed, 49 insertions(+), 31 deletions(-) diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs index d7d21707f40fe..26d2df2e47955 100644 --- a/src/librustc/middle/trans/base.rs +++ b/src/librustc/middle/trans/base.rs @@ -1653,7 +1653,9 @@ pub fn new_fn_ctxt_w_id(ccx: @mut CrateContext, fcx.llenv = unsafe { llvm::LLVMGetParam(llfndecl, fcx.env_arg_pos() as c_uint) }; - fcx.llretptr = Some(make_return_pointer(fcx, substd_output_type)); + if !ty::type_is_nil(substd_output_type) { + fcx.llretptr = Some(make_return_pointer(fcx, substd_output_type)); + } fcx } @@ -1808,7 +1810,7 @@ pub fn build_return_block(fcx: fn_ctxt) { let ret_cx = raw_block(fcx, false, fcx.llreturn); // Return the value if this function immediate; otherwise, return void. - if fcx.has_immediate_return_value { + if fcx.llretptr.is_some() && fcx.has_immediate_return_value { Ret(ret_cx, Load(ret_cx, fcx.llretptr.get())) } else { RetVoid(ret_cx) @@ -2340,8 +2342,7 @@ pub fn create_entry_wrapper(ccx: @mut CrateContext, llvm::LLVMGetParam(llfdecl, env_arg as c_uint) }; let args = ~[llenvarg]; - let llresult = Call(bcx, main_llfn, args); - Store(bcx, llresult, fcx.llretptr.get()); + Call(bcx, main_llfn, args); build_return(bcx); finish_fn(fcx, lltop); diff --git a/src/librustc/middle/trans/cabi.rs b/src/librustc/middle/trans/cabi.rs index acc3293f26783..373980569c343 100644 --- a/src/librustc/middle/trans/cabi.rs +++ b/src/librustc/middle/trans/cabi.rs @@ -179,16 +179,18 @@ impl FnType { } } - let llretval = load_inbounds(bcx, llargbundle, [ 0, arg_tys.len() ]); - let llretval = if self.ret_ty.cast { - let retptr = BitCast(bcx, llretval, T_ptr(self.ret_ty.ty)); - Load(bcx, retptr) - } else { - Load(bcx, llretval) - }; - let llretptr = BitCast(bcx, - bcx.fcx.llretptr.get(), - T_ptr(self.ret_ty.ty)); - Store(bcx, llretval, llretptr); + if bcx.fcx.llretptr.is_some() { + let llretval = load_inbounds(bcx, llargbundle, [ 0, arg_tys.len() ]); + let llretval = if self.ret_ty.cast { + let retptr = BitCast(bcx, llretval, T_ptr(self.ret_ty.ty)); + Load(bcx, retptr) + } else { + Load(bcx, llretval) + }; + let llretptr = BitCast(bcx, + bcx.fcx.llretptr.get(), + T_ptr(self.ret_ty.ty)); + Store(bcx, llretval, llretptr); + } } } diff --git a/src/librustc/middle/trans/cabi_x86.rs b/src/librustc/middle/trans/cabi_x86.rs index 53af55bca6ca2..da7c0d5272da8 100644 --- a/src/librustc/middle/trans/cabi_x86.rs +++ b/src/librustc/middle/trans/cabi_x86.rs @@ -60,6 +60,11 @@ impl ABIInfo for X86_ABIInfo { cast: false, ty: T_void(), }; + } else if !ret_def { + ret_ty = LLVMType { + cast: false, + ty: T_void() + }; } return FnType { diff --git a/src/librustc/middle/trans/closure.rs b/src/librustc/middle/trans/closure.rs index c368ab5c9bdf5..a8ff0bf54faf8 100644 --- a/src/librustc/middle/trans/closure.rs +++ b/src/librustc/middle/trans/closure.rs @@ -297,7 +297,10 @@ pub fn build_closure(bcx0: block, // the right thing): let ret_true = match bcx.fcx.loop_ret { Some((_, retptr)) => retptr, - None => bcx.fcx.llretptr.get() + None => match bcx.fcx.llretptr { + None => C_null(T_ptr(T_nil())), + Some(retptr) => retptr, + } }; let ret_casted = PointerCast(bcx, ret_true, T_ptr(T_nil())); let ret_datum = Datum {val: ret_casted, ty: ty::mk_nil(), diff --git a/src/librustc/middle/trans/controlflow.rs b/src/librustc/middle/trans/controlflow.rs index fe3e2940907b7..55436e5946ef5 100644 --- a/src/librustc/middle/trans/controlflow.rs +++ b/src/librustc/middle/trans/controlflow.rs @@ -298,24 +298,27 @@ pub fn trans_cont(bcx: block, label_opt: Option) -> block { pub fn trans_ret(bcx: block, e: Option<@ast::expr>) -> block { let _icx = bcx.insn_ctxt("trans_ret"); let mut bcx = bcx; - let retptr = match copy bcx.fcx.loop_ret { + let dest = match copy bcx.fcx.loop_ret { Some((flagptr, retptr)) => { // This is a loop body return. Must set continue flag (our retptr) // to false, return flag to true, and then store the value in the // parent's retptr. Store(bcx, C_bool(true), flagptr); Store(bcx, C_bool(false), bcx.fcx.llretptr.get()); - match e { + expr::SaveIn(match e { Some(x) => PointerCast(bcx, retptr, T_ptr(type_of(bcx.ccx(), expr_ty(bcx, x)))), None => retptr - } + }) + } + None => match bcx.fcx.llretptr { + None => expr::Ignore, + Some(retptr) => expr::SaveIn(retptr), } - None => bcx.fcx.llretptr.get() }; match e { Some(x) => { - bcx = expr::trans_into(bcx, x, expr::SaveIn(retptr)); + bcx = expr::trans_into(bcx, x, dest); } _ => () } diff --git a/src/librustc/middle/trans/foreign.rs b/src/librustc/middle/trans/foreign.rs index 40fa44d92ba49..9516a8b83ab35 100644 --- a/src/librustc/middle/trans/foreign.rs +++ b/src/librustc/middle/trans/foreign.rs @@ -127,7 +127,7 @@ fn shim_types(ccx: @mut CrateContext, id: ast::node_id) -> ShimTypes { llsig: llsig, ret_def: ret_def, bundle_ty: bundle_ty, - shim_fn_ty: T_fn([T_ptr(bundle_ty)], T_nil()), + shim_fn_ty: T_fn([T_ptr(bundle_ty)], T_void()), fn_ty: fn_ty } } @@ -170,7 +170,7 @@ fn build_shim_fn_(ccx: @mut CrateContext, tie_up_header_blocks(fcx, lltop); let ret_cx = raw_block(fcx, false, fcx.llreturn); - Ret(ret_cx, C_null(T_nil())); + RetVoid(ret_cx); return llshimfn; } @@ -530,8 +530,10 @@ pub fn trans_foreign_mod(ccx: @mut CrateContext, store_inbounds(bcx, llargval, llargbundle, [0u, i]); } - let llretptr = bcx.fcx.llretptr.get(); - store_inbounds(bcx, llretptr, llargbundle, [0u, n]); + + for bcx.fcx.llretptr.iter().advance |&retptr| { + store_inbounds(bcx, retptr, llargbundle, [0u, n]); + } } fn build_ret(bcx: block, @@ -539,8 +541,10 @@ pub fn trans_foreign_mod(ccx: @mut CrateContext, llargbundle: ValueRef) { let _icx = bcx.insn_ctxt("foreign::wrap::build_ret"); let arg_count = shim_types.fn_sig.inputs.len(); - let llretptr = load_inbounds(bcx, llargbundle, [0, arg_count]); - Store(bcx, Load(bcx, llretptr), bcx.fcx.llretptr.get()); + for bcx.fcx.llretptr.iter().advance |&retptr| { + let llretptr = load_inbounds(bcx, llargbundle, [0, arg_count]); + Store(bcx, Load(bcx, llretptr), retptr); + } build_return(bcx); } } @@ -1294,7 +1298,7 @@ pub fn trans_foreign_fn(ccx: @mut CrateContext, shim_types: &ShimTypes, llargbundle: ValueRef, llretval: ValueRef) { - if ty::type_is_immediate(shim_types.fn_sig.output) { + if bcx.fcx.llretptr.is_some() && ty::type_is_immediate(shim_types.fn_sig.output) { // Write the value into the argument bundle. let arg_count = shim_types.fn_sig.inputs.len(); let llretptr = load_inbounds(bcx, diff --git a/src/librustc/middle/trans/type_of.rs b/src/librustc/middle/trans/type_of.rs index 268d60d441768..058a6b9f48ef4 100644 --- a/src/librustc/middle/trans/type_of.rs +++ b/src/librustc/middle/trans/type_of.rs @@ -55,7 +55,7 @@ pub fn type_of_fn(cx: &mut CrateContext, inputs: &[ty::t], output: ty::t) atys.push_all(type_of_explicit_args(cx, inputs)); // Use the output as the actual return value if it's immediate. - if output_is_immediate { + if output_is_immediate && !ty::type_is_nil(output) { T_fn(atys, lloutputtype) } else { T_fn(atys, llvm::LLVMVoidTypeInContext(cx.llcx)) @@ -352,7 +352,7 @@ pub fn llvm_type_name(cx: &CrateContext, } pub fn type_of_dtor(ccx: &mut CrateContext, self_ty: ty::t) -> TypeRef { - T_fn([T_ptr(type_of(ccx, self_ty))] /* self */, T_nil()) + T_fn([T_ptr(type_of(ccx, self_ty))] /* self */, T_void()) } pub fn type_of_rooted(ccx: &mut CrateContext, t: ty::t) -> TypeRef { @@ -364,5 +364,5 @@ pub fn type_of_rooted(ccx: &mut CrateContext, t: ty::t) -> TypeRef { pub fn type_of_glue_fn(ccx: &CrateContext) -> TypeRef { let tydescpp = T_ptr(T_ptr(ccx.tydesc_type)); - return T_fn([T_ptr(T_nil()), tydescpp, T_ptr(T_i8())], T_nil()); + return T_fn([T_ptr(T_nil()), tydescpp, T_ptr(T_i8())], T_void()); }