Skip to content

Commit

Permalink
auto merge of #7452 : dotdash/rust/self_indirection, r=cmr
Browse files Browse the repository at this point in the history 
Currently we pass all "self" arguments by reference, for the pointer
variants this means that we end up with double indirection which causes
a unnecessary performance hit.

The fix itself is pretty straight-forward and just means that "self"
needs to be handled like any other argument, except for by-value "self"
which still needs to be passed by reference. This is because
non-pointer types can't just be stuffed into the environment slot which
is used to pass "self".

What made things tricky is that there was also a bug in the typechecker
where the method map entries are created. For type impls, that stored
the base type instead of the actual self-type in the method map, e.g.
Foo instead of &Foo for &self. That worked with pass-by-reference, but
fails with pass-by-value which needs the real type.

Code that makes use of methods seems to be about 10% faster with this
change. Also, build times are reduced by about 4%.

Fixes #4355, #4402, #5280, #4406 and #7285
  • Loading branch information
@bors
bors committed Jun 29, 2013
2 parents 439b13f + 765a290 commit df39932
Show file tree
Hide file tree
Showing 8 changed files with 82 additions and 127 deletions.
3 changes: 0 additions & 3 deletions src/librustc/middle/trans/asm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ pub fn trans_inline_asm(bcx: block, ia: &ast::inline_asm) -> block {
callee::trans_arg_expr(bcx,
expr_ty(bcx, out),
ty::ByCopy,
ast::sty_static,
out,
&mut cleanups,
None,
Expand All @@ -57,7 +56,6 @@ pub fn trans_inline_asm(bcx: block, ia: &ast::inline_asm) -> block {
callee::trans_arg_expr(bcx,
expr_ty(bcx, e),
ty::ByCopy,
ast::sty_static,
e,
&mut cleanups,
None,
Expand All @@ -79,7 +77,6 @@ pub fn trans_inline_asm(bcx: block, ia: &ast::inline_asm) -> block {
callee::trans_arg_expr(bcx,
expr_ty(bcx, in),
ty::ByCopy,
ast::sty_static,
in,
&mut cleanups,
None,
Expand Down
29 changes: 14 additions & 15 deletions src/librustc/middle/trans/base.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1626,18 +1626,11 @@ pub fn create_llargs_for_fn_args(cx: fn_ctxt,
let _icx = push_ctxt("create_llargs_for_fn_args");

match self_arg {
impl_self(tt) => {
impl_self(tt, self_mode) => {
cx.llself = Some(ValSelfData {
v: cx.llenv,
t: tt,
is_owned: false
});
}
impl_owned_self(tt) => {
cx.llself = Some(ValSelfData {
v: cx.llenv,
t: tt,
is_owned: true
is_copy: self_mode == ty::ByCopy
});
}
no_self => ()
Expand Down Expand Up @@ -1676,12 +1669,18 @@ pub fn copy_args_to_allocas(fcx: fn_ctxt,

match fcx.llself {
Some(slf) => {
let self_val = PointerCast(bcx, slf.v, type_of(bcx.ccx(), slf.t).ptr_to());
fcx.llself = Some(ValSelfData {v: self_val, ..slf});
let self_val = if slf.is_copy
&& datum::appropriate_mode(slf.t).is_by_value() {
let tmp = BitCast(bcx, slf.v, type_of(bcx.ccx(), slf.t));
let alloc = alloc_ty(bcx, slf.t);
Store(bcx, tmp, alloc);
alloc
} else {
PointerCast(bcx, slf.v, type_of(bcx.ccx(), slf.t).ptr_to())
};

if slf.is_owned {
add_clean(bcx, slf.v, slf.t);
}
fcx.llself = Some(ValSelfData {v: self_val, ..slf});
add_clean(bcx, self_val, slf.t);
}
_ => {}
}
Expand Down Expand Up @@ -1758,7 +1757,7 @@ pub fn tie_up_header_blocks(fcx: fn_ctxt, lltop: BasicBlockRef) {
}
}

pub enum self_arg { impl_self(ty::t), impl_owned_self(ty::t), no_self, }
pub enum self_arg { impl_self(ty::t, ty::SelfMode), no_self, }

// trans_closure: Builds an LLVM function out of a source function.
// If the function closes over its environment a closure will be
Expand Down
52 changes: 19 additions & 33 deletions src/librustc/middle/trans/callee.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ pub struct FnData {
pub struct MethodData {
llfn: ValueRef,
llself: ValueRef,
temp_cleanup: Option<ValueRef>,
self_ty: ty::t,
self_mode: ty::SelfMode,
explicit_self: ast::explicit_self_
}

pub enum CalleeData {
Expand Down Expand Up @@ -615,10 +615,7 @@ pub fn trans_call_inner(in_cx: block,
}
Method(d) => {
// Weird but true: we pass self in the *environment* slot!
let llself = PointerCast(bcx,
d.llself,
Type::opaque_box(ccx).ptr_to());
(d.llfn, llself)
(d.llfn, d.llself)
}
Closure(d) => {
// Closures are represented as (llfn, llclosure) pair:
Expand Down Expand Up @@ -647,11 +644,12 @@ pub fn trans_call_inner(in_cx: block,


// Now that the arguments have finished evaluating, we need to revoke
// the cleanup for the self argument, if it exists
// the cleanup for the self argument
match callee.data {
Method(d) if d.self_mode == ty::ByCopy ||
d.explicit_self == ast::sty_value => {
revoke_clean(bcx, d.llself);
Method(d) => {
for d.temp_cleanup.iter().advance |&v| {
revoke_clean(bcx, v);
}
}
_ => {}
}
Expand Down Expand Up @@ -772,7 +770,6 @@ pub fn trans_args(cx: block,
trans_arg_expr(bcx,
arg_tys[i],
ty::ByCopy,
ast::sty_static,
*arg_expr,
&mut temp_cleanups,
if i == last { ret_flag } else { None },
Expand Down Expand Up @@ -806,18 +803,16 @@ pub enum AutorefArg {
pub fn trans_arg_expr(bcx: block,
formal_arg_ty: ty::t,
self_mode: ty::SelfMode,
ex_self: ast::explicit_self_,
arg_expr: @ast::expr,
temp_cleanups: &mut ~[ValueRef],
ret_flag: Option<ValueRef>,
autoref_arg: AutorefArg) -> Result {
let _icx = push_ctxt("trans_arg_expr");
let ccx = bcx.ccx();

debug!("trans_arg_expr(formal_arg_ty=(%s), explicit_self=%? self_mode=%?, arg_expr=%s, \
debug!("trans_arg_expr(formal_arg_ty=(%s), self_mode=%?, arg_expr=%s, \
ret_flag=%?)",
formal_arg_ty.repr(bcx.tcx()),
ex_self,
self_mode,
arg_expr.repr(bcx.tcx()),
ret_flag.map(|v| bcx.val_to_str(*v)));
Expand Down Expand Up @@ -877,9 +872,15 @@ pub fn trans_arg_expr(bcx: block,
val = arg_datum.to_ref_llval(bcx);
}
DontAutorefArg => {
match (self_mode, ex_self) {
(ty::ByRef, ast::sty_value) => {
debug!("by value self with type %s, storing to scratch",
match self_mode {
ty::ByRef => {
// This assertion should really be valid, but because
// the explicit self code currently passes by-ref, it
// does not hold.
//
//assert !bcx.ccx().maps.moves_map.contains_key(
// &arg_expr.id);
debug!("by ref arg with type %s, storing to scratch",
bcx.ty_to_str(arg_datum.ty));
let scratch = scratch_datum(bcx, arg_datum.ty, false);

Expand All @@ -896,18 +897,7 @@ pub fn trans_arg_expr(bcx: block,

val = scratch.to_ref_llval(bcx);
}
(ty::ByRef, _) => {
// This assertion should really be valid, but because
// the explicit self code currently passes by-ref, it
// does not hold.
//
//assert !bcx.ccx().maps.moves_map.contains_key(
// &arg_expr.id);
debug!("by ref arg with type %s",
bcx.ty_to_str(arg_datum.ty));
val = arg_datum.to_ref_llval(bcx);
}
(ty::ByCopy, _) => {
ty::ByCopy => {
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",
Expand All @@ -930,7 +920,7 @@ pub fn trans_arg_expr(bcx: block,
ByRef(_) => val = scratch.val,
}
} else {
debug!("by copy arg with type %s");
debug!("by copy arg with type %s", bcx.ty_to_str(arg_datum.ty));
match arg_datum.mode {
ByRef(_) => val = Load(bcx, arg_datum.val),
ByValue => val = arg_datum.val,
Expand All @@ -944,10 +934,6 @@ pub fn trans_arg_expr(bcx: block,
if formal_arg_ty != arg_datum.ty {
// this could happen due to e.g. subtyping
let llformal_arg_ty = type_of::type_of_explicit_arg(ccx, &formal_arg_ty);
let llformal_arg_ty = match self_mode {
ty::ByRef => llformal_arg_ty.ptr_to(),
ty::ByCopy => llformal_arg_ty,
};
debug!("casting actual type (%s) to match formal (%s)",
bcx.val_to_str(val), bcx.llty_str(llformal_arg_ty));
val = PointerCast(bcx, val, llformal_arg_ty);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/trans/common.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ pub type ExternMap = HashMap<@str, ValueRef>;
pub struct ValSelfData {
v: ValueRef,
t: ty::t,
is_owned: bool
is_copy: bool,
}

// Here `self_ty` is the real type of the self parameter to this method. It
Expand Down
14 changes: 2 additions & 12 deletions src/librustc/middle/trans/glue.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,12 +425,7 @@ pub fn trans_struct_drop_flag(bcx: block, t: ty::t, v0: ValueRef, dtor_did: ast:
// just consist of the environment (self)
assert_eq!(params.len(), 1);

// Take a reference to the class (because it's using the Drop trait),
// do so now.
let llval = alloca(bcx, val_ty(v0));
Store(bcx, v0, llval);

let self_arg = PointerCast(bcx, llval, params[0]);
let self_arg = PointerCast(bcx, v0, params[0]);
let args = ~[self_arg];

Call(bcx, dtor_addr, args);
Expand Down Expand Up @@ -465,12 +460,7 @@ pub fn trans_struct_drop(mut bcx: block, t: ty::t, v0: ValueRef, dtor_did: ast::
// just consist of the environment (self)
assert_eq!(params.len(), 1);

// Take a reference to the class (because it's using the Drop trait),
// do so now.
let llval = alloca(bcx, val_ty(v0));
Store(bcx, v0, llval);

let self_arg = PointerCast(bcx, llval, params[0]);
let self_arg = PointerCast(bcx, v0, params[0]);
let args = ~[self_arg];

Call(bcx, dtor_addr, args);
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/middle/trans/inline.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use core::prelude::*;

use metadata::csearch;
use middle::astencode;
use middle::trans::base::{push_ctxt,impl_owned_self, impl_self, no_self};
use middle::trans::base::{push_ctxt, impl_self, no_self};
use middle::trans::base::{trans_item, get_item_val, trans_fn};
use middle::trans::common::*;
use middle::ty;
Expand Down Expand Up @@ -114,8 +114,8 @@ pub fn maybe_instantiate_inline(ccx: @mut CrateContext, fn_id: ast::def_id,
debug!("calling inline trans_fn with self_ty %s",
ty_to_str(ccx.tcx, self_ty));
match mth.explicit_self.node {
ast::sty_value => impl_owned_self(self_ty),
_ => impl_self(self_ty),
ast::sty_value => impl_self(self_ty, ty::ByRef),
_ => impl_self(self_ty, ty::ByCopy),
}
}
};
Expand Down
Loading

0 comments on commit df39932

Please sign in to comment.