Avoid double indirection for the "self" arg in methods

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

8 files changed, 57 insertions, 78 deletions

diff --git a/src/librustc/middle/trans/asm.rs b/src/librustc/middle/trans/asm.rs
index a1d1b737f31..81a69dfca24 100644
--- a/src/librustc/middle/trans/asm.rs
+++ b/src/librustc/middle/trans/asm.rs
@@ -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,
@@ -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,
@@ -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,
diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs
index d9fea121346..60f4096cf9e 100644
--- a/src/librustc/middle/trans/base.rs
+++ b/src/librustc/middle/trans/base.rs
@@ -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 => ()
@@ -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);
         }
         _ => {}
     }
@@ -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
diff --git a/src/librustc/middle/trans/callee.rs b/src/librustc/middle/trans/callee.rs
index 821f5c7950f..ffeb4f2754a 100644
--- a/src/librustc/middle/trans/callee.rs
+++ b/src/librustc/middle/trans/callee.rs
@@ -65,7 +65,6 @@ pub struct MethodData {
     temp_cleanup: Option<ValueRef>,
     self_ty: ty::t,
     self_mode: ty::SelfMode,
-    explicit_self: ast::explicit_self_
 }
 
 pub enum CalleeData {
@@ -645,7 +644,7 @@ 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) => {
                 for d.temp_cleanup.iter().advance |&v| {
@@ -771,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 },
@@ -805,7 +803,6 @@ 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>,
@@ -813,10 +810,9 @@ pub fn trans_arg_expr(bcx: block,
     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)));
@@ -876,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);
 
@@ -895,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",
diff --git a/src/librustc/middle/trans/common.rs b/src/librustc/middle/trans/common.rs
index b255f2ca78c..229cacef2fe 100644
--- a/src/librustc/middle/trans/common.rs
+++ b/src/librustc/middle/trans/common.rs
@@ -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
diff --git a/src/librustc/middle/trans/glue.rs b/src/librustc/middle/trans/glue.rs
index 0172b705465..9ca174d3f2d 100644
--- a/src/librustc/middle/trans/glue.rs
+++ b/src/librustc/middle/trans/glue.rs
@@ -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);
@@ -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);
diff --git a/src/librustc/middle/trans/inline.rs b/src/librustc/middle/trans/inline.rs
index 11c02f165b6..0d7b5d5ed5a 100644
--- a/src/librustc/middle/trans/inline.rs
+++ b/src/librustc/middle/trans/inline.rs
@@ -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;
@@ -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),
                       }
                   }
               };
diff --git a/src/librustc/middle/trans/meth.rs b/src/librustc/middle/trans/meth.rs
index 3884a2d03f6..845ca34ce54 100644
--- a/src/librustc/middle/trans/meth.rs
+++ b/src/librustc/middle/trans/meth.rs
@@ -20,6 +20,7 @@ use middle::trans::build::*;
 use middle::trans::callee::*;
 use middle::trans::callee;
 use middle::trans::common::*;
+use middle::trans::datum::*;
 use middle::trans::expr::{SaveIn, Ignore};
 use middle::trans::expr;
 use middle::trans::glue;
@@ -107,10 +108,8 @@ pub fn trans_method(ccx: @mut CrateContext,
         debug!("calling trans_fn with self_ty %s",
                self_ty.repr(ccx.tcx));
         match method.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),
         }
       }
     };
@@ -138,7 +137,6 @@ pub fn trans_self_arg(bcx: block,
     trans_arg_expr(bcx,
                    self_ty,
                    mentry.self_mode,
-                   mentry.explicit_self,
                    base,
                    temp_cleanups,
                    None,
@@ -205,7 +203,6 @@ pub fn trans_method_callee(bcx: block,
                     temp_cleanup: temp_cleanups.head_opt().map(|&v| *v),
                     self_ty: node_id_type(bcx, this.id),
                     self_mode: mentry.self_mode,
-                    explicit_self: mentry.explicit_self
                 })
             }
         }
@@ -438,7 +435,6 @@ pub fn trans_monomorphized_callee(bcx: block,
                   temp_cleanup: temp_cleanups.head_opt().map(|&v| *v),
                   self_ty: node_id_type(bcx, base.id),
                   self_mode: mentry.self_mode,
-                  explicit_self: mentry.explicit_self
               })
           }
       }
@@ -568,7 +564,8 @@ pub fn trans_trait_callee_from_llval(bcx: block,
     // necessary:
     let mut llself;
     debug!("(translating trait callee) loading second index from pair");
-    let llbox = Load(bcx, GEPi(bcx, llpair, [0u, abi::trt_field_box]));
+    let llboxptr = GEPi(bcx, llpair, [0u, abi::trt_field_box]);
+    let llbox = Load(bcx, llboxptr);
 
     // Munge `llself` appropriately for the type of `self` in the method.
     match explicit_self {
@@ -580,8 +577,6 @@ pub fn trans_trait_callee_from_llval(bcx: block,
                                 called on objects");
         }
         ast::sty_region(*) => {
-            // As before, we need to pass a pointer to a pointer to the
-            // payload.
             match store {
                 ty::BoxTraitStore |
                 ty::UniqTraitStore => {
@@ -596,7 +591,7 @@ pub fn trans_trait_callee_from_llval(bcx: block,
             // Bump the reference count on the box.
             debug!("(translating trait callee) callee type is `%s`",
                    bcx.ty_to_str(callee_ty));
-            bcx = glue::take_ty(bcx, llbox, callee_ty);
+            glue::incr_refcnt_of_boxed(bcx, llbox);
 
             // Pass a pointer to the box.
             match store {
@@ -610,12 +605,15 @@ pub fn trans_trait_callee_from_llval(bcx: block,
                 ty::UniqTraitStore => llself = llbox,
                 _ => bcx.tcx().sess.bug("~self receiver with non-~Trait")
             }
+
+            zero_mem(bcx, llboxptr, ty::mk_opaque_box(bcx.tcx()));
         }
     }
 
-    let llscratch = alloca(bcx, val_ty(llself));
-    Store(bcx, llself, llscratch);
-    llself = PointerCast(bcx, llscratch, Type::opaque_box(ccx).ptr_to());
+    llself = PointerCast(bcx, llself, Type::opaque_box(ccx).ptr_to());
+    let scratch = scratch_datum(bcx, ty::mk_opaque_box(bcx.tcx()), false);
+    Store(bcx, llself, scratch.val);
+    scratch.add_clean(bcx);
 
     // Load the function from the vtable and cast it to the expected type.
     debug!("(translating trait callee) loading method");
@@ -630,11 +628,10 @@ pub fn trans_trait_callee_from_llval(bcx: block,
         bcx: bcx,
         data: Method(MethodData {
             llfn: mptr,
-            llself: llself,
-            temp_cleanup: None,
-            self_ty: ty::mk_opaque_box(bcx.tcx()),
-            self_mode: ty::ByRef,
-            explicit_self: explicit_self
+            llself: scratch.to_value_llval(bcx),
+            temp_cleanup: Some(scratch.val),
+            self_ty: scratch.ty,
+            self_mode: ty::ByCopy,
             /* XXX: Some(llbox) */
         })
     };
diff --git a/src/librustc/middle/typeck/check/method.rs b/src/librustc/middle/typeck/check/method.rs
index 4bebca3c9a8..2fb8209ecf2 100644
--- a/src/librustc/middle/typeck/check/method.rs
+++ b/src/librustc/middle/typeck/check/method.rs
@@ -976,9 +976,7 @@ impl<'self> LookupContext<'self> {
         let fty = ty::mk_bare_fn(tcx, ty::BareFnTy {sig: fn_sig, ..bare_fn_ty});
         debug!("after replacing bound regions, fty=%s", self.ty_to_str(fty));
 
-        // FIXME(#7411): We always pass self by-ref since we stuff it in the environment slot.
-        // Eventually that should not be the case
-        let self_mode = ty::ByRef;
+        let self_mode = get_mode_from_explicit_self(candidate.method_ty.explicit_self);
 
         // before we only checked whether self_ty could be a subtype
         // of rcvr_ty; now we actually make it so (this may cause
@@ -998,7 +996,7 @@ impl<'self> LookupContext<'self> {
         self.fcx.write_ty(self.callee_id, fty);
         self.fcx.write_substs(self.callee_id, all_substs);
         method_map_entry {
-            self_ty: candidate.rcvr_ty,
+            self_ty: rcvr_ty,
             self_mode: self_mode,
             explicit_self: candidate.method_ty.explicit_self,
             origin: candidate.origin,
@@ -1253,3 +1251,10 @@ impl<'self> LookupContext<'self> {
         self.tcx().sess.bug(s)
     }
 }
+
+pub fn get_mode_from_explicit_self(explicit_self: ast::explicit_self_) -> SelfMode {
+    match explicit_self {
+        sty_value => ty::ByRef,
+        _ => ty::ByCopy,
+    }
+}