Implement typeof in fast compiler.

Implement typeof in fast compiler.

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

[William Hesse, 2009-11-03 16:04:48]

Only on x64 platform right now.  What about when we are inside a function?

[Kevin Millikin (Chromium), 2009-11-03 16:48:09]

I could be wrong, but I thought this should be simpler (and more complicated in
the 'look up in context' case).

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

http://codereview.chromium.org/354027/diff/1/3#newcode1014
Line 1014: Comment cmnt(masm_, "[ UnaryOperationVOID");
OK to put specific comments for each type of operation.  If you do take the
general comment away ("[ UnaryOperation"), because there is no point in having
the two immediately adjacent.  (It wastes space in the disassembly and the reloc
info, and doesn't add anything).

I find something like "UnaryOperation: VOID" or "UnaryOperation (VOID)" more
readable.

http://codereview.chromium.org/354027/diff/1/3#newcode1104
Line 1104: if (proxy != NULL && (rewrite = proxy->var()->rewrite()) == NULL) {
The variable 'rewrite' is assigned here but never used.  I think the whole test
reads better as:

(proxy != NULL && proxy->var()->is_global())

http://codereview.chromium.org/354027/diff/1/3#newcode1106
Line 1106: __ push(CodeGenerator::GlobalObject());
Can't we accomplish the same thing with a simple load?  Couldn't the whole body
of this if be:

__ push(CodeGenerator::GlobalObject);
__ Move(rcx, proxy->name());
Handle<Code> ic = ...unitialized load IC...
__ Call(ic, RelocInfo::CODE_TARGET);
__ push(rax);

Code seems shorter, and doesn't have two separate entries into the runtime to
fetch the property in the case that it is present (the common case?).

http://codereview.chromium.org/354027/diff/1/3#newcode1126
Line 1126: Visit(expr->expression());
Ultimately, we will have to handle variables that have to be looked up in the
context here as well.  Do you have a plan for that?

[William Hesse, 2009-11-04 14:30:08]

Comments addressed, new version uploaded.

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

http://codereview.chromium.org/354027/diff/1/3#newcode1014
Line 1014: Comment cmnt(masm_, "[ UnaryOperationVOID");
On 2009/11/03 16:48:09, Kevin Millikin wrote:
> OK to put specific comments for each type of operation.  If you do take the
> general comment away ("[ UnaryOperation"), because there is no point in having
> the two immediately adjacent.  (It wastes space in the disassembly and the
reloc
> info, and doesn't add anything).
> 
> I find something like "UnaryOperation: VOID" or "UnaryOperation (VOID)" more
> readable.

Done.

http://codereview.chromium.org/354027/diff/1/3#newcode1104
Line 1104: if (proxy != NULL && (rewrite = proxy->var()->rewrite()) == NULL) {
On 2009/11/03 16:48:09, Kevin Millikin wrote:
> The variable 'rewrite' is assigned here but never used.  I think the whole
test
> reads better as:
> 
> (proxy != NULL && proxy->var()->is_global())

Done.  The control flow and tests are difficult to do cleanly.  Please see if
you can simplify the nesting of if statements.

http://codereview.chromium.org/354027/diff/1/3#newcode1106
Line 1106: __ push(CodeGenerator::GlobalObject());
On 2009/11/03 16:48:09, Kevin Millikin wrote:
> Can't we accomplish the same thing with a simple load?  Couldn't the whole
body
> of this if be:
> 
> __ push(CodeGenerator::GlobalObject);
> __ Move(rcx, proxy->name());
> Handle<Code> ic = ...unitialized load IC...
> __ Call(ic, RelocInfo::CODE_TARGET);
> __ push(rax);
> 
> Code seems shorter, and doesn't have two separate entries into the runtime to
> fetch the property in the case that it is present (the common case?).

Done.

http://codereview.chromium.org/354027/diff/1/3#newcode1126
Line 1126: Visit(expr->expression());
On 2009/11/03 16:48:09, Kevin Millikin wrote:
> Ultimately, we will have to handle variables that have to be looked up in the
> context here as well.  Do you have a plan for that?

Yes.  We are catching the Slot::LOOKUP case, and avoiding returning a reference
error from it.  I think no other case can return a reference error.

[William Hesse, 2009-11-04 15:25:27]

Finished.  All platforms tested.  Lookup Slots are not used in the fast compiler
yet, but are implemented in this patch, creating temporarily dead code.

[Kevin Millikin (Chromium), 2009-11-04 16:51:17]

http://codereview.chromium.org/354027/diff/7001/8002
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/354027/diff/7001/8002#newcode1100
Line 1100: Comment cmnt(masm_, "[ UnaryOperation (TYPEOF)");
Might put an ASSERT that the subexpression is in value context, to match the
other cases of VisitUnaryOperation.

http://codereview.chromium.org/354027/diff/7001/8002#newcode1103
Line 1103: Expression* rewrite = proxy->var()->rewrite();
I think this is clearer (more explicit):

if (proxy != NULL && proxy->var()->is_global()) {
  // Global....
} else if (proxy != NULL &&
           proxy->var()->slot() != NULL &&
           proxy->var()->slot()->type() == Slot::LOOKUP) {
  // Lookup slot ....
} else {
  // Benign.
  Visit(expr->expression());
}

http://codereview.chromium.org/354027/diff/7001/8002#newcode1109
Line 1109: __ Call(ic, RelocInfo::CODE_TARGET);
This deserves a simple comment that you are using a regular load IC (with an
explicit receiver) rather than a contextual load IC (and why).

http://codereview.chromium.org/354027/diff/7001/8002#newcode1116
Line 1116: __ Push(slot->var()->name());
Should be able to write "proxy->name()" like above.  Otherwise I wonder why you
did it differently at this site.

[Kevin Millikin (Chromium), 2009-11-04 17:54:28]

http://codereview.chromium.org/354027/diff/7003/7007
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/354027/diff/7003/7007#newcode1024
Line 1024: __ push(r0);
Because the context is (conveniently) the first argument, you can push both
arguments with

__ stm(db_w, sp, cp.bit() | r0.bit());

http://codereview.chromium.org/354027/diff/7003/7006
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/354027/diff/7003/7006#newcode1020
Line 1020: slot->type() == Slot::LOOKUP) {
Indentation is wierd here and in the other platform files.  But as I suggested
before, I think this whole if should be simplified (to just test the three cases
we care about).

[William Hesse, 2009-11-05 09:49:09]

All comments addressed.

http://codereview.chromium.org/354027/diff/7001/8002
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/354027/diff/7001/8002#newcode1100
Line 1100: Comment cmnt(masm_, "[ UnaryOperation (TYPEOF)");
On 2009/11/04 16:51:17, Kevin Millikin wrote:
> Might put an ASSERT that the subexpression is in value context, to match the
> other cases of VisitUnaryOperation.

Done.

http://codereview.chromium.org/354027/diff/7001/8002#newcode1103
Line 1103: Expression* rewrite = proxy->var()->rewrite();
On 2009/11/04 16:51:17, Kevin Millikin wrote:
> I think this is clearer (more explicit):
> 
> if (proxy != NULL && proxy->var()->is_global()) {
>   // Global....
> } else if (proxy != NULL &&
>            proxy->var()->slot() != NULL &&
>            proxy->var()->slot()->type() == Slot::LOOKUP) {
>   // Lookup slot ....
> } else {
>   // Benign.
>   Visit(expr->expression());
> }

Done.

http://codereview.chromium.org/354027/diff/7001/8002#newcode1109
Line 1109: __ Call(ic, RelocInfo::CODE_TARGET);
On 2009/11/04 16:51:17, Kevin Millikin wrote:
> This deserves a simple comment that you are using a regular load IC (with an
> explicit receiver) rather than a contextual load IC (and why).

Done.

http://codereview.chromium.org/354027/diff/7001/8002#newcode1116
Line 1116: __ Push(slot->var()->name());
On 2009/11/04 16:51:17, Kevin Millikin wrote:
> Should be able to write "proxy->name()" like above.  Otherwise I wonder why
you
> did it differently at this site.

Done.

http://codereview.chromium.org/354027/diff/7003/7007
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/354027/diff/7003/7007#newcode1024
Line 1024: __ push(r0);
On 2009/11/04 17:54:28, Kevin Millikin wrote:
> Because the context is (conveniently) the first argument, you can push both
> arguments with
> 
> __ stm(db_w, sp, cp.bit() | r0.bit());

Done.

http://codereview.chromium.org/354027/diff/7003/7006
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/354027/diff/7003/7006#newcode1020
Line 1020: slot->type() == Slot::LOOKUP) {
On 2009/11/04 17:54:28, Kevin Millikin wrote:
> Indentation is wierd here and in the other platform files.  But as I suggested
> before, I think this whole if should be simplified (to just test the three
cases
> we care about).

Done.

[Kevin Millikin (Chromium), 2009-11-05 10:10:26]

LGTM.

http://codereview.chromium.org/354027/diff/11001/11005
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/354027/diff/11001/11005#newcode1022
Line 1022: // Use a regular load, not a contextual load, to avoid reference
error.
error => errors :)