Adding compound assignments to the top-level compiler.

Adding compound assignments to the top-level compiler.


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

[Kevin Millikin (Chromium), 2009-12-10 15:31:39]

Initial round of comments.

http://codereview.chromium.org/486008/diff/1/4
File src/ast.h (right):

http://codereview.chromium.org/486008/diff/1/4#newcode1245
src/ast.h:1245: return op() != Token::ASSIGN && op() != Token::INIT_VAR; }
Don't forget Token::INIT_CONST.

There is already a dependence on the order of tokens in a few places, so you
could just write:

return op() > Token::ASSIGN

and add a comment in token.h that Assignment::is_compound() also depends on the
order of tokens.

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

http://codereview.chromium.org/486008/diff/1/3#newcode380
src/ia32/fast-codegen-ia32.cc:380: void FastCodeGenerator::Drop(int drop_count)
{
There's already a MacroAssembler::Drop, you could just use that.

http://codereview.chromium.org/486008/diff/1/3#newcode557
src/ia32/fast-codegen-ia32.cc:557: EmitVariableLoad(expr);
Why not pass the expression context to EmitVariableLoad, so it can avoid
add(esp, 4); push(eax) for global variable accesses in value context?

http://codereview.chromium.org/486008/diff/1/3#newcode562
src/ia32/fast-codegen-ia32.cc:562: void
FastCodeGenerator::EmitVariableLoad(VariableProxy* expr) {
It might make more sense for this to take a Variable* instead of a
VariableProxy*.

http://codereview.chromium.org/486008/diff/1/3#newcode848
src/ia32/fast-codegen-ia32.cc:848: void
FastCodeGenerator::EmitVariableLoadOnStack(Assignment* expr) {
It might make more sense for this to take a Variable* instead of an Assignment*.

http://codereview.chromium.org/486008/diff/1/3#newcode855
src/ia32/fast-codegen-ia32.cc:855: void
FastCodeGenerator::EmitNamedPropertyLoadOnStack(Assignment* expr) {
It might make more sense for this to take a Property* (or even, Expression*
object and Handle<String> name).

http://codereview.chromium.org/486008/diff/1/3#newcode864
src/ia32/fast-codegen-ia32.cc:864: void
FastCodeGenerator::EmitKeyedPropertyLoadOnStack(Assignment* expr) {
Ditto.

http://codereview.chromium.org/486008/diff/1/3#newcode871
src/ia32/fast-codegen-ia32.cc:871: void
FastCodeGenerator::EmitCompoundAssignmentOp(Assignment* expr) {
It might make more sense for this to just take the Token::Value.

http://codereview.chromium.org/486008/diff/1/3#newcode876
src/ia32/fast-codegen-ia32.cc:876: __ push(eax);
Don't we actually want the value in the accumulator here (we'll pop it before
the store, right?)?

[fschneider, 2009-12-10 16:48:26]

Addressed comments. 

Introducing an accumulator register for passing 
intermediate results may be better done as a separate change.

http://codereview.chromium.org/486008/diff/1/4
File src/ast.h (right):

http://codereview.chromium.org/486008/diff/1/4#newcode1245
src/ast.h:1245: return op() != Token::ASSIGN && op() != Token::INIT_VAR; }
On 2009/12/10 15:31:39, Kevin Millikin wrote:
> Don't forget Token::INIT_CONST.
> 
> There is already a dependence on the order of tokens in a few places, so you
> could just write:
> 
> return op() > Token::ASSIGN
> 
> and add a comment in token.h that Assignment::is_compound() also depends on
the
> order of tokens.

Done.

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

http://codereview.chromium.org/486008/diff/1/3#newcode380
src/ia32/fast-codegen-ia32.cc:380: void FastCodeGenerator::Drop(int drop_count)
{
On 2009/12/10 15:31:39, Kevin Millikin wrote:
> There's already a MacroAssembler::Drop, you could just use that.

Done.

http://codereview.chromium.org/486008/diff/1/3#newcode557
src/ia32/fast-codegen-ia32.cc:557: EmitVariableLoad(expr);
On 2009/12/10 15:31:39, Kevin Millikin wrote:
> Why not pass the expression context to EmitVariableLoad, so it can avoid
> add(esp, 4); push(eax) for global variable accesses in value context?

Agreed. We just have to rely on the kValue context being 'stack'.

http://codereview.chromium.org/486008/diff/1/3#newcode562
src/ia32/fast-codegen-ia32.cc:562: void
FastCodeGenerator::EmitVariableLoad(VariableProxy* expr) {
On 2009/12/10 15:31:39, Kevin Millikin wrote:
> It might make more sense for this to take a Variable* instead of a
> VariableProxy*.

Done.

http://codereview.chromium.org/486008/diff/1/3#newcode855
src/ia32/fast-codegen-ia32.cc:855: void
FastCodeGenerator::EmitNamedPropertyLoadOnStack(Assignment* expr) {
On 2009/12/10 15:31:39, Kevin Millikin wrote:
> It might make more sense for this to take a Property* (or even, Expression*
> object and Handle<String> name).

Done.

http://codereview.chromium.org/486008/diff/1/3#newcode864
src/ia32/fast-codegen-ia32.cc:864: void
FastCodeGenerator::EmitKeyedPropertyLoadOnStack(Assignment* expr) {
On 2009/12/10 15:31:39, Kevin Millikin wrote:
> Ditto.

Actually I just realized that I don't need any arguments for this one :)

http://codereview.chromium.org/486008/diff/1/3#newcode871
src/ia32/fast-codegen-ia32.cc:871: void
FastCodeGenerator::EmitCompoundAssignmentOp(Assignment* expr) {
On 2009/12/10 15:31:39, Kevin Millikin wrote:
> It might make more sense for this to just take the Token::Value.

Done.

http://codereview.chromium.org/486008/diff/1/3#newcode876
src/ia32/fast-codegen-ia32.cc:876: __ push(eax);
On 2009/12/10 15:31:39, Kevin Millikin wrote:
> Don't we actually want the value in the accumulator here (we'll pop it before
> the store, right?)?

The functions for completing the assignment (EmitVariableAssignment,
EmitNamedPropertyAssignment,...) currently expect the RHS value on top of the
stack. 

But yes, we could change that, and I think it makes sense. I'd prefer to do this
as a separate change.

[fschneider, 2009-12-11 13:28:58]

Added x64 and ARM platform.

[fschneider, 2009-12-11 13:34:23]

Fixed check for compound assignment to also check for INIT_CONST token.

[Kevin Millikin (Chromium), 2009-12-11 14:16:00]

LGTM.