Refactor code for generating assignments in the fast compiler....

Refactor code for generating assignments in the fast compiler.

1. Refactor the structure of VisitAssignment: The existing code is not ideal 
to be extended with support for compound assignments.

2. Reuse common code for keyed property assigments: Now variables rewritten 
to a property (.arguments access) are treated like normal keyed property
assignments. This allows us to remove some code duplication.


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

[fschneider, 2009-12-01 17:24:49]

This change is a starting point for supporting compound assignments and generic
pre-/postfix operations in the fast compiler. The plan is to have these new
constructs as separate upcoming changes.

Feedback is welcome :)

[Kevin Millikin (Chromium), 2009-12-02 09:50:59]

LGTM.  You might still wait for comments from Lasse because I'm kind of out of
the loop right now.

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

http://codereview.chromium.org/456024/diff/1/4#newcode510
src/fast-codegen.cc:510: enum { VARIABLE, NAMED_PROPERTY, KEYED_PROPERTY };
Name this type, eg

enum LhsKind { VARIABLE, NAMED_PROPERTY, KEYED_PROPERTY };
LhsKind kind = VARIABLE;

http://codereview.chromium.org/456024/diff/1/4#newcode513
src/fast-codegen.cc:513: Property* prop = expr->target()->AsProperty();
Perhaps simpler, given an initial kind of VARIABLE:

Property* prop = (var != NULL)
    ? var->AsProperty()
    : expr->target()->AsProperty();
ASSERT(prop != NULL || var != NULL);
if (prop != NULL) {
  kind = (prop->key()->context() == Expression::kUnitialized)
      ? NAMED_PROPERTY
      : KEYED_PROPERTY;
}

But you have to assign the key subexpression for arguments accesses an
expression context of kValue (which you should do anyway).

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

http://codereview.chromium.org/456024/diff/1/2#newcode964
src/ia32/fast-codegen-ia32.cc:964: UNREACHABLE();
Instead of an unreachable else, assert up front (before the first if) what you
want to be true:

ASSERT(var->slot() != NULL || var->is_global());

[Lasse Reichstein, 2009-12-03 09:13:12]

What Kevin said.
LGTM

[fschneider, 2009-12-03 13:43:50]

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

http://codereview.chromium.org/456024/diff/1/4#newcode510
src/fast-codegen.cc:510: enum { VARIABLE, NAMED_PROPERTY, KEYED_PROPERTY };
On 2009/12/02 09:50:59, Kevin Millikin wrote:
> Name this type, eg
> 
> enum LhsKind { VARIABLE, NAMED_PROPERTY, KEYED_PROPERTY };
> LhsKind kind = VARIABLE;
> 

Done.

http://codereview.chromium.org/456024/diff/1/4#newcode513
src/fast-codegen.cc:513: Property* prop = expr->target()->AsProperty();
On 2009/12/02 09:50:59, Kevin Millikin wrote:
> Perhaps simpler, given an initial kind of VARIABLE:
> 
> Property* prop = (var != NULL)
>     ? var->AsProperty()
>     : expr->target()->AsProperty();
> ASSERT(prop != NULL || var != NULL);
> if (prop != NULL) {
>   kind = (prop->key()->context() == Expression::kUnitialized)
>       ? NAMED_PROPERTY
>       : KEYED_PROPERTY;
> }
> 
> But you have to assign the key subexpression for arguments accesses an
> expression context of kValue (which you should do anyway).

For some reason I have to look into yet, they have the (correct) context...
If I understand correctly we can even assert that:
ASSERT(prop != NULL || var != NULL);
ASSERT(prop == NULL || var == NULL);

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

http://codereview.chromium.org/456024/diff/1/2#newcode964
src/ia32/fast-codegen-ia32.cc:964: UNREACHABLE();
On 2009/12/02 09:50:59, Kevin Millikin wrote:
> Instead of an unreachable else, assert up front (before the first if) what you
> want to be true:
> 
> ASSERT(var->slot() != NULL || var->is_global());

Done. (also on x64 and arm)

[fschneider, 2009-12-04 09:54:45]

Another small simplification of the code.

http://codereview.chromium.org/456024/diff/11001/9007
File src/fast-codegen.cc (right):

http://codereview.chromium.org/456024/diff/11001/9007#newcode512
src/fast-codegen.cc:512: Variable* var =
expr->target()->AsVariableProxy()->AsVariable();
In case of a variable rewritten to a property, this expression return NULL:
proxy->AsVariable() returns NULL if (proxy->rewrite_->AsSlot() == NULL). That's
why we set the context of obj and key is set to kValue and not kUninitialized.

[Kevin Millikin (Chromium), 2009-12-04 12:10:51]

LGTM, if you add the asserts mentioned below to be extra defensive.

http://codereview.chromium.org/456024/diff/11001/9007
File src/fast-codegen.cc (right):

http://codereview.chromium.org/456024/diff/11001/9007#newcode512
src/fast-codegen.cc:512: Variable* var =
expr->target()->AsVariableProxy()->AsVariable();
On 2009/12/04 09:54:45, fschneider wrote:
> In case of a variable rewritten to a property, this expression return NULL:
> proxy->AsVariable() returns NULL if (proxy->rewrite_->AsSlot() == NULL).
That's
> why we set the context of obj and key is set to kValue and not kUninitialized.

I see.  Then there is dead code in CodeGenSelector::VisitAssignmnent that should
be removed.

Also, could you check for the places we set contexts on the subexpressions of a
variable rewrite that is a property, and assert that they are either unitialized
before setting or not changed by setting?

[fschneider, 2009-12-04 14:22:06]

http://codereview.chromium.org/456024/diff/11001/9007
File src/fast-codegen.cc (right):

http://codereview.chromium.org/456024/diff/11001/9007#newcode512
src/fast-codegen.cc:512: Variable* var =
expr->target()->AsVariableProxy()->AsVariable();
On 2009/12/04 12:10:51, Kevin Millikin wrote:
> On 2009/12/04 09:54:45, fschneider wrote:
> > In case of a variable rewritten to a property, this expression return NULL:
> > proxy->AsVariable() returns NULL if (proxy->rewrite_->AsSlot() == NULL).
> That's
> > why we set the context of obj and key is set to kValue and not
kUninitialized.
> 
> I see.  Then there is dead code in CodeGenSelector::VisitAssignmnent that
should
> be removed.
> 

Done.


> Also, could you check for the places we set contexts on the subexpressions of
a
> variable rewrite that is a property, and assert that they are either
unitialized
> before setting or not changed by setting?

Done.