Fast-codegen: Arguments object working on all platforms.

Fast-codegen: Arguments object working on all platforms.
This time it's true.

[Lasse Reichstein, 2009-11-19 14:01:50]

Anyone with time to spare (and I mean: don't interrupt your vacation unless you
reall, really want to!) please review this.

[fschneider, 2009-11-23 18:32:45]

This is a large one :) 

First round of comments. I only looked at IA32 code. The things there also apply
for other platforms.

In general we should think also about how we deal with expression contexts in
our code generator once we introduce an accumulator and slot operands for
evaluating expressions.

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

http://codereview.chromium.org/405033/diff/1/5#newcode803
src/compiler.cc:803: BAILOUT("non-global/non-slot/non-property variable");
I think that should not happen, should it?

ASSERT(property != NULL);

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

http://codereview.chromium.org/405033/diff/1/7#newcode88
src/ia32/fast-codegen-ia32.cc:88: // Assert we do not have to copy any
parameters into the context.
Change this comment => Copy parameters into context.

http://codereview.chromium.org/405033/diff/1/7#newcode246
src/ia32/fast-codegen-ia32.cc:246: default:
Do you need default at all? We handle all 4 possible enums for Slot.

http://codereview.chromium.org/405033/diff/1/7#newcode333
src/ia32/fast-codegen-ia32.cc:333: case Expression::kValue:\
extra '\'?

http://codereview.chromium.org/405033/diff/1/7#newcode958
src/ia32/fast-codegen-ia32.cc:958: Move(Expression::kValue, object_slot);
Move(...) can destroy eax, maybe we should pass it as a scratch register
explicitly. Otherwise the caller may not be aware of this.

http://codereview.chromium.org/405033/diff/1/7#newcode965
src/ia32/fast-codegen-ia32.cc:965: __ mov(eax, Operand(esp, 2 * kPointerSize));
This line relies on the fact that the Move(...) before pushes the values on the
stack. This may change in the future. 
Since we are pushing arguments to a call here I think its better to do sth.
like:

__ pop(eax)
__ push(object_slot)
__ push(key_literal)

(and do the corresponding stack cleanup code after the KeyedStoreIC, also use a
different scratch register for evaluating the slots then)

General idea for later refactoring: Maybe we should split the Expression::kValue
into different subcases: e.g. Stack, Register, or Slot. 

i.e. state explicitly where to put the value of an expression.

[Lasse Reichstein, 2009-11-25 12:58:23]

I've added missing implementation of varproxy->property assignment to X64 and
ARM.

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

http://codereview.chromium.org/405033/diff/1/5#newcode803
src/compiler.cc:803: BAILOUT("non-global/non-slot/non-property variable");
I don't think so either.
Changed to ASSERT_NOT_NULL(property);

http://codereview.chromium.org/405033/diff/1/5#newcode807
src/compiler.cc:807: BAILOUT("variable rewritten to property with non slot
object");
I don't think this (or the next) bailout can happen either.
I'll change them to asserts too.

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

http://codereview.chromium.org/405033/diff/1/7#newcode88
src/ia32/fast-codegen-ia32.cc:88: // Assert we do not have to copy any
parameters into the context.
Done

http://codereview.chromium.org/405033/diff/1/7#newcode246
src/ia32/fast-codegen-ia32.cc:246: default:
Apparently so. If I remove it, the compiler complains that control reaches the
end of a non-void function.
I'll add a comment to highlight the fallthrough from LOOKUP to default.

http://codereview.chromium.org/405033/diff/1/7#newcode333
src/ia32/fast-codegen-ia32.cc:333: case Expression::kValue:\
Well spotted.

http://codereview.chromium.org/405033/diff/1/7#newcode958
src/ia32/fast-codegen-ia32.cc:958: Move(Expression::kValue, object_slot);
Done.

http://codereview.chromium.org/405033/diff/1/7#newcode965
src/ia32/fast-codegen-ia32.cc:965: __ mov(eax, Operand(esp, 2 * kPointerSize));
I can see the idea, but in this particular case, if we had different kValue
types, I would use the stack version to put the value on the stack. The context
types here are constants, and are picked so that the result will end up on the
stack.
I will put in a comment to the fact.

[fschneider, 2009-11-26 08:17:05]

LGTM.

I like the extensive unit test program!

http://codereview.chromium.org/405033/diff/3004/5001
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/405033/diff/3004/5001#newcode253
src/arm/fast-codegen-arm.cc:253: Register scratch) {
I'm not sure. It is possible to use the ip register as a fixed scratch register
instead?

http://codereview.chromium.org/405033/diff/3004/5001#newcode581
src/arm/fast-codegen-arm.cc:581: Move(Expression::kValue, object_slot, r0);
Would it make sense to have a Move(Register dst, Slot src)?
In a situation like here we could then use the stm instruction:

Move(r2, object_slot, r0);
__ mov(r1, Operand(key_literal->handle());
__ stm(db_w, sp, r2.bit() | r1.bit());

[Lasse Reichstein, 2009-11-26 10:11:25]

http://codereview.chromium.org/405033/diff/3004/5001
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/405033/diff/3004/5001#newcode253
src/arm/fast-codegen-arm.cc:253: Register scratch) {
The problem is that I want the same signature across all three architectures,
and IA32 doesn't have a default scratch register.
(And I want the same signature, because I want it to be a method of
FastCodeGenerator, so that it has access to function_, otherwise I would have to
pass that as a parameter instead).
Also, I don't want to guarantee that the MacroAssembler function LoadContext
doesn't use ip for something else.

http://codereview.chromium.org/405033/diff/3004/5001#newcode581
src/arm/fast-codegen-arm.cc:581: Move(Expression::kValue, object_slot, r0);
That would make sense. It's more obvious what happens, and the code will
probably be slightly shorter. I'll do this.

[Kevin Millikin (Chromium), 2009-12-02 11:47:39]

Drive by comments, questions, and suggestions.

http://codereview.chromium.org/405033/diff/3005/5018
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/405033/diff/3005/5018#newcode98
src/ia32/fast-codegen-ia32.cc:98: __ mov(Operand(esi,
Context::SlotOffset(slot->index())), eax);
Why can we avoid the write barrier here?

http://codereview.chromium.org/405033/diff/3005/5018#newcode123
src/ia32/fast-codegen-ia32.cc:123: __ mov(ecx, eax);  // Duplicate result.
This seems like more generality (and complication) than we need.  As far as I
can see, 'arguments' is in the stack or we don't need to set it anyway (it's
shadowed) [*].  '.arguments' may be in the stack or the local context.  In
either case, we shouldn't need to duplicate the result.

[*] We would thus break from our existing arguments-shadowing behavior, to match
the spec (and ES5).  That seems desirable.