Fast-codegen: Added support for arguments in functions.

src/arm/fast-codegen-arm.cc

Index: src/arm/fast-codegen-arm.cc
diff --git a/src/arm/fast-codegen-arm.cc b/src/arm/fast-codegen-arm.cc
index 56e7ac3671585100e4bc6ac166948ca6dac33aa0..3b5e58e60f3c6c20e55eabae813b6f3fc10f5b04 100644
--- a/src/arm/fast-codegen-arm.cc
+++ b/src/arm/fast-codegen-arm.cc
@@ -71,10 +71,40 @@ void FastCodeGenerator::Generate(FunctionLiteral* fun) {
     }
   }
 
+  bool function_in_register = true;
+
+  Variable* arguments = fun->scope()->arguments()->AsVariable();
+  if (arguments != NULL) {
+    // Function uses arguments object.
+    Comment cmnt(masm_, "[ Allocate arguments object");
+    __ mov(r3, r1);
+    // Receiver is located above the frame pointer, return address and

[Kevin Millikin (Chromium), 2009/11/12 14:23:13]

Like Bill, I think you should avoid "above" and "below" in stack comments, or
else be consistent with the rest of the codegen comments (usually, we push
something "on top" of something else and thus it is "above", regardless of
address).

Receiver is parameter number -1.

[Lasse Reichstein, 2009/11/13 08:54:37]

I committed this comment: "Receiver is just before the parameters on the
caller's stack."
(and the number it comments is "StandardFrameConstants::kCallerSPOffset +
fun->num_parameters() * kPointerSize".

How does this read?

+    // parameters.
+    __ add(r2, fp, Operand((fun->num_parameters() + 2) * kPointerSize));
+    __ mov(r1, Operand(Smi::FromInt(fun->num_parameters())));
+    __ stm(db_w, sp, r1.bit() | r2.bit() | r3.bit());

[Kevin Millikin (Chromium), 2009/11/12 14:23:13]

When using stm to push arguments, list the registers in argument order like you
were making a function call; ie, r3, r2, r1 here.

[Lasse Reichstein, 2009/11/13 08:54:37]

Will fix.

+
+    // Arguments to ArgumentsAccessStub:
+    //   function, receiver address, parameter count.
+    // The stub will rewrite receiever and parameter count if the previous
+    // stack frame was an arguments adapter frame.
+    ArgumentsAccessStub stub(ArgumentsAccessStub::NEW_OBJECT);
+    __ CallStub(&stub);
+    Slot* arguments_slot = arguments->slot();
+    __ str(r0, MemOperand(fp, SlotOffset(arguments_slot)));
+    Slot* dot_arguments_slot =
+        fun->scope()->arguments_shadow()->AsVariable()->slot();
+    __ str(r0, MemOperand(fp, SlotOffset(dot_arguments_slot)));
+    function_in_register = false;

[Kevin Millikin (Chromium), 2009/11/12 14:23:13]

Isn't function_in_register == (arguments != NULL)?  I find it more
straightforward to just use that fact down below.

[Lasse Reichstein, 2009/11/13 08:54:37]

It is. I preferred it this way because it avoided interleaving the two,
otherwise independent, blocks.

If anyone wants to add something between them later, they will need to know that
the meaning of "arguments != NULL" is that the function is no longer in register
r1, as well as why that is the case.

+  }
+
   // Possibly allocate a local context.
   if (fun->scope()->num_heap_slots() > 0) {
     Comment cmnt(masm_, "[ Allocate local context");
     // Argument to NewContext is the function, still in r1.

[Kevin Millikin (Chromium), 2009/11/12 14:23:13]

Comment is wrong.

[Lasse Reichstein, 2009/11/13 08:54:37]

Fixed.

+    if (!function_in_register) {
+      __ ldr(r1, MemOperand(fp, JavaScriptFrameConstants::kFunctionOffset));
+    }
     __ push(r1);
     __ CallRuntime(Runtime::kNewContext, 1);
     // Context is returned in both r0 and cp.  It replaces the context
@@ -432,7 +462,7 @@ void FastCodeGenerator::VisitVariableProxy(VariableProxy* expr) {
     Handle<Code> ic(Builtins::builtin(Builtins::LoadIC_Initialize));
     __ Call(ic, RelocInfo::CODE_TARGET_CONTEXT);
     DropAndMove(expr->context(), r0);
-  } else {
+  } else if (rewrite->AsSlot() != NULL) {
     Slot* slot = rewrite->AsSlot();
     ASSERT_NE(NULL, slot);
     switch (slot->type()) {
@@ -474,6 +504,13 @@ void FastCodeGenerator::VisitVariableProxy(VariableProxy* expr) {
         UNREACHABLE();
         break;
     }
+  } else {
+    // The parameter variable has been rewritten into an explict access to
+    // the arguments object.

[Kevin Millikin (Chromium), 2009/11/12 14:23:13]

Here we don't know or care that it's the arguments object, do we?

I could easily see us changing the way we do variable resolution and rewrites,
and then this remote comment would make no sense.

[Lasse Reichstein, 2009/11/13 08:54:37]

True. It's currently the only reason, but we could prepare for other reasons for
this rewrite (or other expression rewrites, removing the assertion that it's a
property).
I'll reword the comment.

+    Property* property = rewrite->AsProperty();
+    ASSERT_NOT_NULL(property);
+    ASSERT_EQ(expr->context(), property->context());
+    Visit(property);
   }
 }