Enable writes and reads of context slots in fast compiler.

Enable writes and reads of context slots in fast compiler.

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

[William Hesse, 2009-11-05 16:21:59]

Only on x64 so far.

[Kevin Millikin (Chromium), 2009-11-05 16:53:50]

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

http://codereview.chromium.org/360054/diff/1/2#newcode651
Line 651: if (var->mode() == Variable::CONST) {
You should not commit this code.  Let the non-global declaration change land
first?

http://codereview.chromium.org/360054/diff/1/2#newcode885
Line 885: // We support plain non-compound assignments to properties, parameters
and
Change this comment.

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

http://codereview.chromium.org/360054/diff/1/4#newcode91
Line 91: // UNREACHABLE();
Do not commit this.

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

http://codereview.chromium.org/360054/diff/1/3#newcode345
Line 345: if (rewrite == NULL) {
Actually should add ASSERT(expr->var()->is_global()), at least for
documentation.

http://codereview.chromium.org/360054/diff/1/3#newcode370
Line 370: __ movq(rax, rsi);
I liked the other structure, from the optimizing backend, a little better.  It
just seems wrong to emit this move in the case that the context chain has length
zero.

http://codereview.chromium.org/360054/diff/1/3#newcode379
Line 379: #ifdef DEBUG
Don't use #ifdef DEBUG to control whether to emit extra code.  Use if
(FLAG_debug_code).

In general should avoid producing different generated code for debug and release
builds (or at least, make it possible to generate the same code).  Sometimes
(usually) you want to use a debugger to debug the code generated in release
mode.

http://codereview.chromium.org/360054/diff/1/3#newcode704
Line 704: __ movq(rax, rsi);
Same comment as above.

http://codereview.chromium.org/360054/diff/1/3#newcode713
Line 713: #ifdef DEBUG
Same comment as above.

http://codereview.chromium.org/360054/diff/1/3#newcode722
Line 722: if (expr->context() != Expression::kEffect) {
How about

if (expr->context() == Expression::kValue) {
  __ push(rcx);
} else if (expr->context() != Expression::kEffect) {
  __ movq(rdx, rcx);
}
... // RecordWrite
if (expr->context() != Expression::kEffect &&
    expr->context() != Expression::kValue) {
...

[William Hesse, 2009-11-06 13:53:30]

Now working on all platforms.  Erik, can you look at the arm code inside
FLAG_debug_code checks - it is failing.

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

http://codereview.chromium.org/360054/diff/1/3#newcode345
Line 345: if (rewrite == NULL) {
On 2009/11/05 16:53:50, Kevin Millikin wrote:
> Actually should add ASSERT(expr->var()->is_global()), at least for
> documentation.

Done.

http://codereview.chromium.org/360054/diff/1/3#newcode370
Line 370: __ movq(rax, rsi);
On 2009/11/05 16:53:50, Kevin Millikin wrote:
> I liked the other structure, from the optimizing backend, a little better.  It
> just seems wrong to emit this move in the case that the context chain has
length
> zero.

Done.

http://codereview.chromium.org/360054/diff/1/3#newcode379
Line 379: #ifdef DEBUG
On 2009/11/05 16:53:50, Kevin Millikin wrote:
> Don't use #ifdef DEBUG to control whether to emit extra code.  Use if
> (FLAG_debug_code).
> 
> In general should avoid producing different generated code for debug and
release
> builds (or at least, make it possible to generate the same code).  Sometimes
> (usually) you want to use a debugger to debug the code generated in release
> mode.

Done.

http://codereview.chromium.org/360054/diff/1/3#newcode704
Line 704: __ movq(rax, rsi);
On 2009/11/05 16:53:50, Kevin Millikin wrote:
> Same comment as above.

Done.

http://codereview.chromium.org/360054/diff/1/3#newcode713
Line 713: #ifdef DEBUG
On 2009/11/05 16:53:50, Kevin Millikin wrote:
> Same comment as above.

Optimized in length > 1 case.  Because RecordWrite destroys its register
arguments, we still need to move rsi to rax in the length 0 case.

http://codereview.chromium.org/360054/diff/1/3#newcode722
Line 722: if (expr->context() != Expression::kEffect) {
On 2009/11/05 16:53:50, Kevin Millikin wrote:
> How about
> 
> if (expr->context() == Expression::kValue) {
>   __ push(rcx);
> } else if (expr->context() != Expression::kEffect) {
>   __ movq(rdx, rcx);
> }
> ... // RecordWrite
> if (expr->context() != Expression::kEffect &&
>     expr->context() != Expression::kValue) {
> ...

Done.

[William Hesse, 2009-11-09 08:53:27]

Now tested and working on all platforms.  I added the code to go from a context
to its function context back in - it looks like it may be necessary.  The code
for declarations that does not include it is probably ok because the
declarations are at the beginning of the function.

[fschneider, 2009-11-09 09:27:17]

LGTM.

http://codereview.chromium.org/360054/diff/5005/4009
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/360054/diff/5005/4009#newcode828
Line 828: __ tst(r0, Operand(kSmiTagMask));
Do we have to test for SMI before the write barrier here? We don't do this in
all cases where we emit the write barrier.
Maybe this is an optimization we may want to add later?

http://codereview.chromium.org/360054/diff/5005/4006
File src/compiler.cc (right):

http://codereview.chromium.org/360054/diff/5005/4006#newcode771
Line 771: //    BAILOUT("FunctionLiteral does not allow lazy compilation");
See my other change that enables FunctionLiterals

[William Hesse, 2009-11-09 14:02:16]

http://codereview.chromium.org/360054/diff/5005/4009
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/360054/diff/5005/4009#newcode828
Line 828: __ tst(r0, Operand(kSmiTagMask));
On 2009/11/09 09:27:17, fschneider wrote:
> Do we have to test for SMI before the write barrier here? We don't do this in
> all cases where we emit the write barrier.
> Maybe this is an optimization we may want to add later?

Added comment explaining this.  We should add the smi check to RecordWrite.