Fast-codegen: Added support for arguments in functions.

Fast-codegen: Added support for arguments in functions.
Functions using "arguments" have their arguments object created on entry.
Also added support for variables rewritten into argument object property access.

[Lasse Reichstein, 2009-11-12 09:35:57]

Please review.

[William Hesse, 2009-11-12 09:56:49]

LGTM, with comments addressed.

http://codereview.chromium.org/384078/diff/1/3
File src/compiler.cc (right):

http://codereview.chromium.org/384078/diff/1/3#newcode604
Line 604: // return NORMAL;
Remove.

http://codereview.chromium.org/384078/diff/1/3#newcode795
Line 795: // A rewrite of NULL indicates a global variable or explict arguments
access.
Lie!

http://codereview.chromium.org/384078/diff/1/3#newcode807
Line 807: Property* property = rewrite->AsProperty();
Put a comment, indicating what this code is for.

http://codereview.chromium.org/384078/diff/1/4
File src/fast-codegen.cc (right):

http://codereview.chromium.org/384078/diff/1/4#newcode132
Line 132: void FastCodeGenerator::VisitCondition(Expression* expression,
This should be a separate change.  Also, all places where it can be used should
be rewritten to use it.

http://codereview.chromium.org/384078/diff/1/7
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/384078/diff/1/7#newcode84
Line 84: // Receiver is located above the frame pointer, return address and
I think we would say "below" here.  Instead, say that the receiver is parameter
0, to avoid the issue.  Do we have a ParameterOperand(0) function?

http://codereview.chromium.org/384078/diff/1/7#newcode91
Line 91: // The stub will rewrite receiever and parameter count if the previous
receiver

http://codereview.chromium.org/384078/diff/1/7#newcode95
Line 95: Slot* arguments_slot = arguments->slot();
This variable name isn't much shorter than the expression it replaces, and it is
used only once.

And why don't we have a SlotOperand function?

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

Some drive bys.  I'm surprised that putting state on variable rewrites works. 
In general, I don't think we should do it.

http://codereview.chromium.org/384078/diff/1/2
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/384078/diff/1/2#newcode81
Line 81: // Receiver is located above the frame pointer, return address and
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.

http://codereview.chromium.org/384078/diff/1/2#newcode85
Line 85: __ stm(db_w, sp, r1.bit() | r2.bit() | r3.bit());
When using stm to push arguments, list the registers in argument order like you
were making a function call; ie, r3, r2, r1 here.

http://codereview.chromium.org/384078/diff/1/2#newcode98
Line 98: function_in_register = false;
Isn't function_in_register == (arguments != NULL)?  I find it more
straightforward to just use that fact down below.

http://codereview.chromium.org/384078/diff/1/2#newcode104
Line 104: // Argument to NewContext is the function, still in r1.
Comment is wrong.

http://codereview.chromium.org/384078/diff/1/2#newcode509
Line 509: // the arguments object.
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.

http://codereview.chromium.org/384078/diff/1/3
File src/compiler.cc (right):

http://codereview.chromium.org/384078/diff/1/3#newcode811
Line 811: property->set_context(context_);
I'm surprised that this works.

For a variable in a scope, all proxies should point to the same variable and
thus the rewrite is shared.  At codegen time, you will just get whatever is the
last context set in the selector, right?

I would think that we do not (ever) set the context or other code gen state on
rewrites, but instead on the proxy itself.  Then in the case of visiting a
variable proxy at code gen time, compile it based on the proxy's state.

http://codereview.chromium.org/384078/diff/1/5
File src/fast-codegen.h (right):

http://codereview.chromium.org/384078/diff/1/5#newcode73
Line 73: void VisitCondition(Expression* expression, Label* on_true, Label*
on_false);
Please do not call this Visit, because it's not part of the visitor.

Something like "EmitTest", "EmitAsTest", "EmitCondition" (though not all
expressions in test context are really "conditions").

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

I'll look into whether putting context on rewritten variable's expressions is
reasonable (and what to do if it isn't - because then how do we compile the
expression it's rewritten to?).

http://codereview.chromium.org/384078/diff/1/2
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/384078/diff/1/2#newcode81
Line 81: // Receiver is located above the frame pointer, return address and
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?

http://codereview.chromium.org/384078/diff/1/2#newcode85
Line 85: __ stm(db_w, sp, r1.bit() | r2.bit() | r3.bit());
Will fix.

http://codereview.chromium.org/384078/diff/1/2#newcode98
Line 98: function_in_register = false;
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.

http://codereview.chromium.org/384078/diff/1/2#newcode104
Line 104: // Argument to NewContext is the function, still in r1.
Fixed.

http://codereview.chromium.org/384078/diff/1/2#newcode509
Line 509: // the arguments object.
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.

http://codereview.chromium.org/384078/diff/1/3
File src/compiler.cc (right):

http://codereview.chromium.org/384078/diff/1/3#newcode604
Line 604: // return NORMAL;
On 2009/11/12 09:56:49, William Hesse wrote:
> Remove.

Done.

http://codereview.chromium.org/384078/diff/1/3#newcode795
Line 795: // A rewrite of NULL indicates a global variable or explict arguments
access.
Truthified.

http://codereview.chromium.org/384078/diff/1/3#newcode807
Line 807: Property* property = rewrite->AsProperty();
Added

http://codereview.chromium.org/384078/diff/1/4
File src/fast-codegen.cc (right):

http://codereview.chromium.org/384078/diff/1/4#newcode132
Line 132: void FastCodeGenerator::VisitCondition(Expression* expression,
Removed for now.

http://codereview.chromium.org/384078/diff/1/5
File src/fast-codegen.h (right):

http://codereview.chromium.org/384078/diff/1/5#newcode73
Line 73: void VisitCondition(Expression* expression, Label* on_true, Label*
on_false);
I'll call it something else if I revisit the idea.

http://codereview.chromium.org/384078/diff/1/7
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/384078/diff/1/7#newcode84
Line 84: // Receiver is located above the frame pointer, return address and
Using "above" and "below" wrt stacks is the stuff religious wars are started
over. Rephrasing completely is probably better.
The same probably goes for parameter numbering (I'd say parameters are counted
from 0, so the receiver is parameter -1).
I think I'll go for "before the parameters on the stack".
We don't have a ParameterOperand, probably since it will require the number of
parameters as an argument as well, which reduces its immediate appeal).

http://codereview.chromium.org/384078/diff/1/7#newcode91
Line 91: // The stub will rewrite receiever and parameter count if the previous
fixed

http://codereview.chromium.org/384078/diff/1/7#newcode95
Line 95: Slot* arguments_slot = arguments->slot();
I'll inline it.
We probably don't have the SlotOperand function because it would need to go in
the macro-assembler (because its return type is Operand on intel and MemOperand
on ARM), but it would require the code of SlotOffset which resides in codegen.
maybe we could move SlotOffset to the macro assembler(s) as well.