Added pre- and postfix count operations to top-level compiler....

Added general pre- and postfix count operations to top-level compiler.

Until now we only supported postfix operations on global variables.
This change add generic count operations to the top-level compiler.

I tried to re-use code from the code generator used for assignment expressions
where possible.


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

[fschneider, 2009-12-14 15:05:53]

Here's the IA32 part for pre-/postfix operations for review.

The code definitely has potential for refactoring into something nicer. I hope
with new IC calling conventions this code will get a lot cleaner.

(x64 and ARM will follow after first round of feedback)

[Kevin Millikin (Chromium), 2009-12-14 17:06:24]

Seems about as simple as it can be.

It will also be nicer to allow the generated code to propagate results in an
accumulator register.

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

http://codereview.chromium.org/496009/diff/1/5#newcode1028
src/compiler.cc:1028: BAILOUT("Lookup slot");
Maybe change the string to mention count operation, so it doesn't overlap with
another bailout string.

http://codereview.chromium.org/496009/diff/1/5#newcode1032
src/compiler.cc:1032: ASSERT(prop->obj()->context() ==
Expression::kUninitialized ||
This assert is actually in ProcessExpression, so we don't need it here.

http://codereview.chromium.org/496009/diff/1/5#newcode1051
src/compiler.cc:1051: BAILOUT("non-variable/non-property assignment");
Change this string too.

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

http://codereview.chromium.org/496009/diff/1/3#newcode254
src/fast-codegen.h:254: // Load a value from a named property and push the
result on the stack.
"named" ==> "keyed"

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

http://codereview.chromium.org/496009/diff/1/4#newcode1403
src/ia32/fast-codegen-ia32.cc:1403: switch (assign_type) {
Maybe remove the repeated code with:

if (assign_type == VARIABLE) {
  EmitVariableLoad(...);
} else {
  // Reserve space for result of postfix operation.
  Visit(prop->obj());
  if (assign_type == NAMED_PROPERTY) {
    EmitNamedPropertyLoad(...);
  } else {
    Visit(prop->key();
    EmitKeyedPropertyLoad(...);
  }
}

http://codereview.chromium.org/496009/diff/1/4#newcode1479
src/ia32/fast-codegen-ia32.cc:1479:
EmitVariableAssignment(expr->expression()->AsVariableProxy()->var(),
It means repeating the call to EmitVariableAssignment (but avoids testing
is_postfix twice).  I like:

if (expr->is_postfix()) {
  EmitVariableAssignment(..., Expression::kEffect);
  // Postfix-specific handling.
} else {
  EmitVariableAssignment(..., expr->context());
}

http://codereview.chromium.org/496009/diff/1/4#newcode1525
src/ia32/fast-codegen-ia32.cc:1525: __ Drop(1);  // Result in on the stack under
the receiver.
"in" ==> "is"

http://codereview.chromium.org/496009/diff/1/4#newcode1526
src/ia32/fast-codegen-ia32.cc:1526: if (expr->context() != Expression::kEffect)
{
You might consider

if (expr->context() != Expression::kEffect &&
    expr->context() != Expression::kValue) { ... }

It matches what you do for variables, and avoids pop/push for value contexts.

http://codereview.chromium.org/496009/diff/1/4#newcode1542
src/ia32/fast-codegen-ia32.cc:1542: if (expr->context() != Expression::kEffect)
{
Same comment.

[fschneider, 2009-12-15 12:04:24]

Addresses comments. 

+ Factored out common code sequence "Move TOS -> expression context"

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

http://codereview.chromium.org/496009/diff/1/5#newcode1028
src/compiler.cc:1028: BAILOUT("Lookup slot");
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> Maybe change the string to mention count operation, so it doesn't overlap with
> another bailout string.

Done.

http://codereview.chromium.org/496009/diff/1/5#newcode1032
src/compiler.cc:1032: ASSERT(prop->obj()->context() ==
Expression::kUninitialized ||
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> This assert is actually in ProcessExpression, so we don't need it here.

Done. I also removed the redundandt ASSERT from a couple of other places.

http://codereview.chromium.org/496009/diff/1/5#newcode1051
src/compiler.cc:1051: BAILOUT("non-variable/non-property assignment");
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> Change this string too.

Done.

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

http://codereview.chromium.org/496009/diff/1/3#newcode254
src/fast-codegen.h:254: // Load a value from a named property and push the
result on the stack.
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> "named" ==> "keyed"

Done.

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

http://codereview.chromium.org/496009/diff/1/4#newcode1403
src/ia32/fast-codegen-ia32.cc:1403: switch (assign_type) {
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> Maybe remove the repeated code with:
> 
> if (assign_type == VARIABLE) {
>   EmitVariableLoad(...);
> } else {
>   // Reserve space for result of postfix operation.
>   Visit(prop->obj());
>   if (assign_type == NAMED_PROPERTY) {
>     EmitNamedPropertyLoad(...);
>   } else {
>     Visit(prop->key();
>     EmitKeyedPropertyLoad(...);
>   }
> }

Done.

http://codereview.chromium.org/496009/diff/1/4#newcode1479
src/ia32/fast-codegen-ia32.cc:1479:
EmitVariableAssignment(expr->expression()->AsVariableProxy()->var(),
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> It means repeating the call to EmitVariableAssignment (but avoids testing
> is_postfix twice).  I like:
> 
> if (expr->is_postfix()) {
>   EmitVariableAssignment(..., Expression::kEffect);
>   // Postfix-specific handling.
> } else {
>   EmitVariableAssignment(..., expr->context());
> }  

Done.

http://codereview.chromium.org/496009/diff/1/4#newcode1525
src/ia32/fast-codegen-ia32.cc:1525: __ Drop(1);  // Result in on the stack under
the receiver.
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> "in" ==> "is"

Done.

http://codereview.chromium.org/496009/diff/1/4#newcode1526
src/ia32/fast-codegen-ia32.cc:1526: if (expr->context() != Expression::kEffect)
{
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> You might consider
> 
> if (expr->context() != Expression::kEffect &&
>     expr->context() != Expression::kValue) { ... }
> 
> It matches what you do for variables, and avoids pop/push for value contexts.

Agreed. I introduced a new common code sequence MoveTOS which moves the top of
stack into a destination context.

http://codereview.chromium.org/496009/diff/1/4#newcode1542
src/ia32/fast-codegen-ia32.cc:1542: if (expr->context() != Expression::kEffect)
{
On 2009/12/14 17:06:24, Kevin Millikin wrote:
> Same comment.

Done.

[fschneider, 2009-12-29 14:09:25]

Uploaded new version. Please take a look at the x64 and ARM code.

Changes:
+ Added x64 and ARM code.
+ Merged with latest revision.

[Kevin Millikin (Chromium), 2010-01-04 12:48:37]

LGTM.

I can't quite figure out the logic behind where SetSourcePosition is being
called.  As far as I know, we need it for every statement and every IC call.  In
the optimizing backend, we have it for every statement and every expression
where an IC call is possible.

It looks like you are putting it in every expression where an IC call is
inevitable.  In general, I like that, but then it can be removed from places
like VisitAssignment.

http://codereview.chromium.org/496009/diff/14001/14008
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/496009/diff/14001/14008#newcode917
src/arm/fast-codegen-arm.cc:917: Handle<Code>
ic(Builtins::builtin(Builtins::StoreIC_Initialize));
SetSourcePosition.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1012
src/arm/fast-codegen-arm.cc:1012: Move(context, r3);
Indentation looks off here.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1042
src/arm/fast-codegen-arm.cc:1042: Handle<Code>
ic(Builtins::builtin(Builtins::StoreIC_Initialize));
SetSourcePosition.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1072
src/arm/fast-codegen-arm.cc:1072: Handle<Code>
ic(Builtins::builtin(Builtins::KeyedStoreIC_Initialize));
SetSourcePosition.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1412
src/arm/fast-codegen-arm.cc:1412: Handle<Code>
ic(Builtins::builtin(Builtins::LoadIC_Initialize));
SetSourcePosition.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1461
src/arm/fast-codegen-arm.cc:1461: } else  {
Too many spaces after else?

http://codereview.chromium.org/496009/diff/14001/14008#newcode1517
src/arm/fast-codegen-arm.cc:1517: __ CallRuntime(Runtime::kNumberAdd, 2);
For the INC case, we can use stm to push the two arguments to the runtime call. 
Didn't we decide that was better?

http://codereview.chromium.org/496009/diff/14001/14008#newcode1551
src/arm/fast-codegen-arm.cc:1551: } break;
We usually put break inside the braces.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1563
src/arm/fast-codegen-arm.cc:1563: } break;
And here.

http://codereview.chromium.org/496009/diff/14001/14005
File src/compiler.cc (right):

http://codereview.chromium.org/496009/diff/14001/14005#newcode653
src/compiler.cc:653: // expression context state.
Comment can go away.

http://codereview.chromium.org/496009/diff/14001/14006
File src/fast-codegen.cc (right):

http://codereview.chromium.org/496009/diff/14001/14006#newcode634
src/fast-codegen.cc:634: SetSourcePosition(expr->position());
No need?

http://codereview.chromium.org/496009/diff/14001/14004
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/496009/diff/14001/14004#newcode1527
src/ia32/fast-codegen-ia32.cc:1527: } break;
Break inside braces.

http://codereview.chromium.org/496009/diff/14001/14004#newcode1542
src/ia32/fast-codegen-ia32.cc:1542: } break;
And here.

[fschneider, 2010-01-04 13:55:34]

For now I'm leaving the calls to SetSourcePosition as is, but but yes, we should
do it in a consistent way in the future. I'll look at this in a separate change.

http://codereview.chromium.org/496009/diff/14001/14008
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/496009/diff/14001/14008#newcode1012
src/arm/fast-codegen-arm.cc:1012: Move(context, r3);
On 2010/01/04 12:48:38, Kevin Millikin wrote:
> Indentation looks off here.

Done.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1461
src/arm/fast-codegen-arm.cc:1461: } else  {
On 2010/01/04 12:48:38, Kevin Millikin wrote:
> Too many spaces after else?

Done.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1517
src/arm/fast-codegen-arm.cc:1517: __ CallRuntime(Runtime::kNumberAdd, 2);
On 2010/01/04 12:48:38, Kevin Millikin wrote:
> For the INC case, we can use stm to push the two arguments to the runtime
call. 
> Didn't we decide that was better?

In this case I didn't use it since the previous call to TO_NUMBER returns the
result already in r0. We'd have to move the first argument into a higher-number
register to use stm (or change the order of the arguments for the runtime call
NumberAdd.)

http://codereview.chromium.org/496009/diff/14001/14008#newcode1551
src/arm/fast-codegen-arm.cc:1551: } break;
On 2010/01/04 12:48:38, Kevin Millikin wrote:
> We usually put break inside the braces.

Done.

http://codereview.chromium.org/496009/diff/14001/14008#newcode1563
src/arm/fast-codegen-arm.cc:1563: } break;
On 2010/01/04 12:48:38, Kevin Millikin wrote:
> And here.

Done.

http://codereview.chromium.org/496009/diff/14001/14005
File src/compiler.cc (right):

http://codereview.chromium.org/496009/diff/14001/14005#newcode653
src/compiler.cc:653: // expression context state.
On 2010/01/04 12:48:38, Kevin Millikin wrote:
> Comment can go away.

Done.

http://codereview.chromium.org/496009/diff/14001/14004
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/496009/diff/14001/14004#newcode1527
src/ia32/fast-codegen-ia32.cc:1527: } break;
On 2010/01/04 12:48:38, Kevin Millikin wrote:
> Break inside braces.

Done.

http://codereview.chromium.org/496009/diff/14001/14004#newcode1542
src/ia32/fast-codegen-ia32.cc:1542: } break;
On 2010/01/04 12:48:38, Kevin Millikin wrote:
> And here.

Done.