Support for object literals in fast compiler....

Support for object literals in fast compiler.

I also added more unit tests for literals.

Right now, the fast compiler produces code very similar to
the existing code generator. We may consider different ways to 
further compact the generated code for top-level code.

ARM always goes through a runtime function to initialize computed
properties in an object literal whereas IA32 and x64 use StoreIC.


Committed: http://code.google.com/p/v8/source/detail?r=3129

[fschneider, 2009-10-22 14:31:12]

This is the IA32 part.

[Kevin Millikin (Chromium), 2009-10-22 15:13:15]

Initial ideas.

http://codereview.chromium.org/316009/diff/1001/11
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/316009/diff/1001/11#newcode247
Line 247: switch (property->kind()) {
You could avoid duplicating the rather annoying result_saved check.

ObjectLiteral::Property::Kind kind = property->kind();
if (kind == ObjectLiteral::Property::CONSTANT) continue;
if (kind == ObjectLiteral::Property::MATERIALIZED_LITERAL &&
    CompileTimeValue::IsCompileTimeValue(property->value())) {
  continue;
}

// Here you know you will emit code...
if (!result_saved) {
  __ push(eax);
  result_saved = true;
}
switch (kind) {
 ....
}

http://codereview.chromium.org/316009/diff/1001/11#newcode268
Line 268: // Drop the result left on the stack.
Here you're dropping the receiver.  As long as store ICs never patch the
receiver, there is no reason to have duplicated it in the first place.

http://codereview.chromium.org/316009/diff/1001/11#newcode282
Line 282: ASSERT(property->key()->location().is_temporary());
Can't this ASSERT fail if the key is a number or a non-symbol string? (The
latter might not be currently possible.)

http://codereview.chromium.org/316009/diff/1001/11#newcode286
Line 286: // Ignore the result.
The result of the runtime call is the object.  It might be better to avoid
duplicating it on the stack and get the result back in eax.  You would have to
push it before visiting the subexpressions (but you do that anyway) and you
could get rid of the whole result_saved thing.

[fschneider, 2009-10-23 13:13:46]

Added ARM and x64 support.

I did a few changes and cleanup to simplify the rather complicated code.

http://codereview.chromium.org/316009/diff/1001/11
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/316009/diff/1001/11#newcode247
Line 247: switch (property->kind()) {
On 2009/10/22 15:13:15, Kevin Millikin wrote:
> You could avoid duplicating the rather annoying result_saved check.
> 
> ObjectLiteral::Property::Kind kind = property->kind();
> if (kind == ObjectLiteral::Property::CONSTANT) continue;
> if (kind == ObjectLiteral::Property::MATERIALIZED_LITERAL &&
>     CompileTimeValue::IsCompileTimeValue(property->value())) {
>   continue;
> }
> 
> // Here you know you will emit code...
> if (!result_saved) {
>   __ push(eax);
>   result_saved = true;
> }
> switch (kind) {
>  ....
> }
> 
> 

Done.

http://codereview.chromium.org/316009/diff/1001/11#newcode268
Line 268: // Drop the result left on the stack.
On 2009/10/22 15:13:15, Kevin Millikin wrote:
> Here you're dropping the receiver.  As long as store ICs never patch the
> receiver, there is no reason to have duplicated it in the first place.

Done.

http://codereview.chromium.org/316009/diff/1001/11#newcode282
Line 282: ASSERT(property->key()->location().is_temporary());
On 2009/10/22 15:13:15, Kevin Millikin wrote:
> Can't this ASSERT fail if the key is a number or a non-symbol string? (The
> latter might not be currently possible.)

Done.

http://codereview.chromium.org/316009/diff/1001/11#newcode286
Line 286: // Ignore the result.
On 2009/10/22 15:13:15, Kevin Millikin wrote:
> The result of the runtime call is the object.  It might be better to avoid
> duplicating it on the stack and get the result back in eax.  You would have to
> push it before visiting the subexpressions (but you do that anyway) and you
> could get rid of the whole result_saved thing.

Done.

[Kevin Millikin (Chromium), 2009-10-24 07:30:08]

I like the cleanup/simplification of ia32 and x64.  They look good.  I think the
ARM should be cleaned up in the same way.

There are some coding style comments.

One other comment is that it's probably a good idea to try to make the comments
the same (where reasonable) on all three platforms.  We don't want someone to
have to read the ia32 version to get the good comments.  We also don't want
people to wonder if differences are important or superficial.

http://codereview.chromium.org/316009/diff/7002/7008
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/316009/diff/7002/7008#newcode228
Line 228: // If so, jump to the deferred code.
The comment about deferred code is out of date.

http://codereview.chromium.org/316009/diff/7002/7008#newcode242
Line 242: __ push(r0);
There's an opportunity to combine the three pushes into a stm by choosing a
higher register index than r1 for the literals array.  I think you can just swap
the use of r1 and r2 in the code before and including the undefined check.

http://codereview.chromium.org/316009/diff/7002/7008#newcode255
Line 255: __ push(r0);
I think you can skip this push and use code more like the ia32, keeping the
object in r0 for each spin of the code generation loop.

http://codereview.chromium.org/316009/diff/7002/7008#newcode268
Line 268: case ObjectLiteral::Property::PROTOTYPE: {
No need for block.

http://codereview.chromium.org/316009/diff/7002/7008#newcode275
Line 275: Visit(value);
Assert this is on stack (location is_temporary for now).

http://codereview.chromium.org/316009/diff/7002/7008#newcode277
Line 277: __ ldr(r0, MemOperand(sp));
The value restored to r0 from the stack is the same as the one already in r0
returned by the runtime.

http://codereview.chromium.org/316009/diff/7002/7008#newcode281
Line 281: case ObjectLiteral::Property::GETTER: {
No need for block.

http://codereview.chromium.org/316009/diff/7002/7008#newcode292
Line 292: Visit(value);
Assert this is on stack.

http://codereview.chromium.org/316009/diff/7002/7008#newcode294
Line 294: __ ldr(r0, MemOperand(sp));
Same comment.  I don't think there is a need to save the object on the stack or
restore it.

http://codereview.chromium.org/316009/diff/7002/7008#newcode299
Line 299: if (expr->location().is_nowhere()) {
Here you would do nothing for nowhere and push r0 for temporary.

http://codereview.chromium.org/316009/diff/7002/7004
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/316009/diff/7002/7004#newcode245
Line 245: bool result_saved = false;
I think the code below reads a little better if you call this 'object_in_eax' or
'object_in_reg' or something instead.  (Ie, reverse the sense of the boolean.)

http://codereview.chromium.org/316009/diff/7002/7004#newcode256
Line 256: if (!result_saved) {
if (object_in_eax) {
  __ push(eax);
  object_in_eax = false;
}

http://codereview.chromium.org/316009/diff/7002/7004#newcode261
Line 261: case ObjectLiteral::Property::MATERIALIZED_LITERAL:  // fall through
We indent case labels two spaces (and their bodies a further two spaces).

http://codereview.chromium.org/316009/diff/7002/7004#newcode263
Line 263: case ObjectLiteral::Property::COMPUTED: {
No need for a block around this case.

http://codereview.chromium.org/316009/diff/7002/7004#newcode276
Line 276: case ObjectLiteral::Property::PROTOTYPE: {
No need for a block around this one.

http://codereview.chromium.org/316009/diff/7002/7004#newcode285
Line 285: result_saved = false;
object_in_eax = true;

http://codereview.chromium.org/316009/diff/7002/7004#newcode289
Line 289: case ObjectLiteral::Property::GETTER: {
No need for block.

[fschneider, 2009-10-26 12:17:33]

Another round of cleanup:

- Fixes on ARM
- Fixed indentation.
- Removed unnecessary {}-blocks.
- Fixed calls to runtime to restore eax.
- Made comment more consistent.

http://codereview.chromium.org/316009/diff/7002/7008
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/316009/diff/7002/7008#newcode228
Line 228: // If so, jump to the deferred code.
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> The comment about deferred code is out of date.

Done.

http://codereview.chromium.org/316009/diff/7002/7008#newcode242
Line 242: __ push(r0);
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> There's an opportunity to combine the three pushes into a stm by choosing a
> higher register index than r1 for the literals array.  I think you can just
swap
> the use of r1 and r2 in the code before and including the undefined check.

Done.

http://codereview.chromium.org/316009/diff/7002/7008#newcode255
Line 255: __ push(r0);
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> I think you can skip this push and use code more like the ia32, keeping the
> object in r0 for each spin of the code generation loop.

Added a result_saved variable so that we only save the result on the stack if it
is needed.

http://codereview.chromium.org/316009/diff/7002/7008#newcode268
Line 268: case ObjectLiteral::Property::PROTOTYPE: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for block.

Done.

http://codereview.chromium.org/316009/diff/7002/7008#newcode275
Line 275: Visit(value);
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> Assert this is on stack (location is_temporary for now).

Done.

http://codereview.chromium.org/316009/diff/7002/7008#newcode277
Line 277: __ ldr(r0, MemOperand(sp));
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> The value restored to r0 from the stack is the same as the one already in r0
> returned by the runtime.

Done. SetProperty return the value not the receiver in r0. So we have to save r0
in this case. I fixed this also on ia32 and x64.

http://codereview.chromium.org/316009/diff/7002/7008#newcode281
Line 281: case ObjectLiteral::Property::GETTER: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for block.

Done.

http://codereview.chromium.org/316009/diff/7002/7008#newcode292
Line 292: Visit(value);
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> Assert this is on stack.

Done.

http://codereview.chromium.org/316009/diff/7002/7008#newcode294
Line 294: __ ldr(r0, MemOperand(sp));
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> Same comment.  I don't think there is a need to save the object on the stack
or
> restore it.

Same comment as for SetProperty applies here.

http://codereview.chromium.org/316009/diff/7002/7008#newcode299
Line 299: if (expr->location().is_nowhere()) {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> Here you would do nothing for nowhere and push r0 for temporary.

Done. I added a result_saved variable that determines if we have to pop() here
or do nothing.

http://codereview.chromium.org/316009/diff/7002/7004
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/316009/diff/7002/7004#newcode245
Line 245: bool result_saved = false;
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> I think the code below reads a little better if you call this 'object_in_eax'
or
> 'object_in_reg' or something instead.  (Ie, reverse the sense of the boolean.)

I left result_saved unchanged for now to be consistent with the other
Visit-functions where we use the same name.

http://codereview.chromium.org/316009/diff/7002/7004#newcode256
Line 256: if (!result_saved) {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> if (object_in_eax) {
>   __ push(eax);
>   object_in_eax = false;
> }

Unchanged. See my other comment about the result_saved variable.

http://codereview.chromium.org/316009/diff/7002/7004#newcode261
Line 261: case ObjectLiteral::Property::MATERIALIZED_LITERAL:  // fall through
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> We indent case labels two spaces (and their bodies a further two spaces).

Done. (also for x64)

http://codereview.chromium.org/316009/diff/7002/7004#newcode263
Line 263: case ObjectLiteral::Property::COMPUTED: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for a block around this case.

Done. (also for x64)

http://codereview.chromium.org/316009/diff/7002/7004#newcode276
Line 276: case ObjectLiteral::Property::PROTOTYPE: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for a block around this one.

Done.

http://codereview.chromium.org/316009/diff/7002/7004#newcode285
Line 285: result_saved = false;
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> object_in_eax = true;

Unchanged. See my other comment about the result_saved variable.

http://codereview.chromium.org/316009/diff/7002/7004#newcode289
Line 289: case ObjectLiteral::Property::GETTER: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for block.

Done. (also for x64)

[Kevin Millikin (Chromium), 2009-10-26 13:00:52]

LGTM.

http://codereview.chromium.org/316009/diff/7002/7008
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/316009/diff/7002/7008#newcode277
Line 277: __ ldr(r0, MemOperand(sp));
On 2009/10/26 12:17:33, fschneider wrote:
> On 2009/10/24 07:30:08, Kevin Millikin wrote:
> > The value restored to r0 from the stack is the same as the one already in r0
> > returned by the runtime.
> 
> Done. SetProperty return the value not the receiver in r0. So we have to save
r0
> in this case. I fixed this also on ia32 and x64.

Good catch.