Adding declaration of global variables and functions in new compiler....

Adding declaration of global variables and functions in new compiler.
Adding calls to global functions to the new compiler.


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

[Kevin Millikin (Chromium), 2009-10-19 19:50:27]

A bunch of style comments in the first round.

http://codereview.chromium.org/302002/diff/4001/4004
File src/compiler.cc (right):

http://codereview.chromium.org/302002/diff/4001/4004#newcode461
Line 461: if (!CheckDeclarations(fun)) {
You can make this function set the has_supported_syntax flag and do:

has_supported_syntax_ = true;  // Here not below.
VisitDeclarations(fun->scope()->declarations()); // Renamed, see next comment.
if (!has_supported_syntax_) return NORMAL;

Also, it might be good to put the fast checks (like materialized literals) first
to get an early cheap bailout for now.

http://codereview.chromium.org/302002/diff/4001/4004#newcode466
Line 466: if (FLAG_trace_bailout) PrintF("Unsupported literal\n");
Two space indent.

http://codereview.chromium.org/302002/diff/4001/4004#newcode494
Line 494: bool CodeGenSelector::CheckDeclarations(FunctionLiteral* fun) {
Here you can override AstVisitor::VisitDeclarations:

void CodeGenSelector::VisitDeclarations(ZoneList<Declaration*>* decls) {
  for (int i = 0; i < decls->length(); i++) {
    Visit(decls->at(i));
    CHECK_BAILOUT;
  }
}

http://codereview.chromium.org/302002/diff/4001/4004#newcode517
Line 517: void CodeGenSelector::VisitDeclaration(Declaration* decl) {
And here you can provide a definition:

void CodeGenSelector::VisitDeclaration(Declaration* decl) {
  Variable* var = decl->proxy()->var();
  if (!var->is_global() || var->mode() == Variable::CONST) {
    BAILOUT("reason");
  }
}

http://codereview.chromium.org/302002/diff/4001/4004#newcode734
Line 734: BAILOUT("CallRuntime");
The rule we use is that the body does not have braces surrounding it if and only
if it is on the same line as the if.

This one probably fits on the same line with the if.

http://codereview.chromium.org/302002/diff/4001/4005
File src/fast-codegen.cc (right):

http://codereview.chromium.org/302002/diff/4001/4005#newcode72
Line 72: void FastCodeGenerator::ProcessDeclarations() {
There is an AstVisitor::VisitDeclarations that you can override.

http://codereview.chromium.org/302002/diff/4001/4005#newcode102
Line 102: if ((slot != NULL && slot->type() == Slot::LOOKUP) ||
!var->is_global()) {
I've never really liked this if with an empty body, even though it parallels
exactly the loop above.  What do you think of:

if ((slot == NULL || slot->type() == Slot::LOOKUP) && var->is_global()) { ...

and then no else?

http://codereview.chromium.org/302002/diff/4001/4002
File src/fast-codegen.h (right):

http://codereview.chromium.org/302002/diff/4001/4002#newcode64
Line 64: Operand ContextOperand(Register context, int index) const {
This function and the next are platform-specific.  The type 'Operand' is not
correct on all platforms---it should be MemOperand on ARM, and the next function
specifically mentions register esi.

I think I'd prefer to keep them out of here for now.  We should be able to make
the corresponding CodeGenerator functions static and call them for now at the
cost of writing CodeGenerator:: at the call sites.

It would be nice if we could find a way to keep a single header file interface,
perhaps by adding a platform specific CodeGenHelper or something.

http://codereview.chromium.org/302002/diff/4001/4003
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/302002/diff/4001/4003#newcode67
Line 67: Comment cmnt(masm_, "[ declarations");
Consider moving the comment into VisitDeclarations.  If not, it should have a
block scope (a) so there isn't a problem with another identifier named cmnt in
the same scope and (b) because the Comment class will print a matching "]" when
the destructor is run.

http://codereview.chromium.org/302002/diff/4001/4003#newcode122
Line 122: // Push function name.
This comment seems superfluous.

http://codereview.chromium.org/302002/diff/4001/4003#newcode124
Line 124: // Push global object (receiver)
Period at the end of comment.

http://codereview.chromium.org/302002/diff/4001/4003#newcode126
Line 126: // Push the arguments.
This comment is probably unnecessary.

http://codereview.chromium.org/302002/diff/4001/4003#newcode211
Line 211: // Push the boilerplate on the stack.
Comment doesn't seem necessary.  Instead, "// Create a new closure." belongs
here not below.

[fschneider, 2009-10-20 12:17:31]

http://codereview.chromium.org/302002/diff/4001/4003
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/302002/diff/4001/4003#newcode110
Line 110: void FastCodeGenerator::VisitBlock(Block* stmt) {
Add comment here:
Comment cmnt(masm_, "[Block");

Add debug info for here:
SetStatementPosition(stmt);

[fschneider, 2009-10-20 13:44:50]

I addressed the comments. Added x64 and arm code.

http://codereview.chromium.org/302002/diff/4001/4004
File src/compiler.cc (right):

http://codereview.chromium.org/302002/diff/4001/4004#newcode461
Line 461: if (!CheckDeclarations(fun)) {
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> You can make this function set the has_supported_syntax flag and do:
> 
> has_supported_syntax_ = true;  // Here not below.
> VisitDeclarations(fun->scope()->declarations()); // Renamed, see next comment.
> if (!has_supported_syntax_) return NORMAL;
> 
> Also, it might be good to put the fast checks (like materialized literals)
first
> to get an early cheap bailout for now.

Agreed. DONE.

http://codereview.chromium.org/302002/diff/4001/4004#newcode466
Line 466: if (FLAG_trace_bailout) PrintF("Unsupported literal\n");
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> Two space indent.

DONE.

http://codereview.chromium.org/302002/diff/4001/4004#newcode494
Line 494: bool CodeGenSelector::CheckDeclarations(FunctionLiteral* fun) {
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> Here you can override AstVisitor::VisitDeclarations:
> 
> void CodeGenSelector::VisitDeclarations(ZoneList<Declaration*>* decls) {
>   for (int i = 0; i < decls->length(); i++) {
>     Visit(decls->at(i));
>     CHECK_BAILOUT;
>   }
> }

DONE.

http://codereview.chromium.org/302002/diff/4001/4004#newcode517
Line 517: void CodeGenSelector::VisitDeclaration(Declaration* decl) {
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> And here you can provide a definition:
> 
> void CodeGenSelector::VisitDeclaration(Declaration* decl) {
>   Variable* var = decl->proxy()->var();
>   if (!var->is_global() || var->mode() == Variable::CONST) {
>     BAILOUT("reason");
>   }
> }

DONE.

http://codereview.chromium.org/302002/diff/4001/4004#newcode734
Line 734: BAILOUT("CallRuntime");
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> The rule we use is that the body does not have braces surrounding it if and
only
> if it is on the same line as the if.
> 
> This one probably fits on the same line with the if.

DONE.

http://codereview.chromium.org/302002/diff/4001/4005
File src/fast-codegen.cc (right):

http://codereview.chromium.org/302002/diff/4001/4005#newcode72
Line 72: void FastCodeGenerator::ProcessDeclarations() {
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> There is an AstVisitor::VisitDeclarations that you can override.

Yes, but the function ProcessDeclarations traverses the declarations list twice.
I just copied the old code generators function here. But we may refactor it
later.

http://codereview.chromium.org/302002/diff/4001/4005#newcode102
Line 102: if ((slot != NULL && slot->type() == Slot::LOOKUP) ||
!var->is_global()) {
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> I've never really liked this if with an empty body, even though it parallels
> exactly the loop above.  What do you think of:
> 
> if ((slot == NULL || slot->type() == Slot::LOOKUP) && var->is_global()) { ...
> 
> and then no else?

Agreed.

I guess it should be:
 
if ((slot == NULL || slot->type() != Slot::LOOKUP) && var->is_global()) {...

then.

http://codereview.chromium.org/302002/diff/4001/4002
File src/fast-codegen.h (right):

http://codereview.chromium.org/302002/diff/4001/4002#newcode64
Line 64: Operand ContextOperand(Register context, int index) const {
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> This function and the next are platform-specific.  The type 'Operand' is not
> correct on all platforms---it should be MemOperand on ARM, and the next
function
> specifically mentions register esi.
> 
> I think I'd prefer to keep them out of here for now.  We should be able to
make
> the corresponding CodeGenerator functions static and call them for now at the
> cost of writing CodeGenerator:: at the call sites.
> 
> It would be nice if we could find a way to keep a single header file
interface,
> perhaps by adding a platform specific CodeGenHelper or something.

Yes, agreed. I moved ContextOperand to ia32/fastcodegen-ia32.cc as a static
function for now.

http://codereview.chromium.org/302002/diff/4001/4003
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/302002/diff/4001/4003#newcode67
Line 67: Comment cmnt(masm_, "[ declarations");
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> Consider moving the comment into VisitDeclarations.  If not, it should have a
> block scope (a) so there isn't a problem with another identifier named cmnt in
> the same scope and (b) because the Comment class will print a matching "]"
when
> the destructor is run.

DONE.

http://codereview.chromium.org/302002/diff/4001/4003#newcode122
Line 122: // Push function name.
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> This comment seems superfluous.

Done.

http://codereview.chromium.org/302002/diff/4001/4003#newcode124
Line 124: // Push global object (receiver)
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> Period at the end of comment.

Done.

http://codereview.chromium.org/302002/diff/4001/4003#newcode126
Line 126: // Push the arguments.
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> This comment is probably unnecessary.

Done.

http://codereview.chromium.org/302002/diff/4001/4003#newcode211
Line 211: // Push the boilerplate on the stack.
On 2009/10/19 19:50:27, Kevin Millikin wrote:
> Comment doesn't seem necessary.  Instead, "// Create a new closure." belongs
> here not below.

Done.

[Kevin Millikin (Chromium), 2009-10-20 14:49:12]

Another round of comments.

http://codereview.chromium.org/302002/diff/6001/6007
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/302002/diff/6001/6007#newcode89
Line 89: Comment cmnt(masm_, "[ declarations");
Format this the same as the other blocks: comment on the same line as the brace
and capitalize Declarations.

http://codereview.chromium.org/302002/diff/6001/6007#newcode90
Line 90: ProcessDeclarations();
Override AstVisitor::VisitDeclarations?

http://codereview.chromium.org/302002/diff/6001/6007#newcode125
Line 125: Comment cmnt(masm_, "[Block");
Space after "[" in comment string for uniformity.

http://codereview.chromium.org/302002/diff/6001/6007#newcode131
Line 131: static MemOperand ContextOperand(Register context, int index) {
Get rid of this function and just call CodeGenerator::ContextOperand.

http://codereview.chromium.org/302002/diff/6001/6007#newcode136
Line 136: void FastCodeGenerator::VisitCall(Call* expr) {
I like to keep these sorted in the same order as the node lists in ast.h (or at
least, all the expressions together and all the statements together).

http://codereview.chromium.org/302002/diff/6001/6007#newcode141
Line 141: 
Blank line not needed.

http://codereview.chromium.org/302002/diff/6001/6007#newcode142
Line 142: ASSERT(var != NULL && !var->is_this() && var->is_global());
Might add !var->is_possibly_eval() to the assert for safety.

If the assert is a conjunction, we usually write each conjunct as a separate
assert (within reason) so it's easy to see which one failed.

http://codereview.chromium.org/302002/diff/6001/6007#newcode147
Line 147: __ ldr(r0, ContextOperand(cp, Context::GLOBAL_INDEX));
Use CodeGenerator::GlobalObject()?

http://codereview.chromium.org/302002/diff/6001/6007#newcode167
Line 167: __ add(sp, sp, Operand(1 * kPointerSize));
__ pop() (with no argument) does the same thing and is shorter.

http://codereview.chromium.org/302002/diff/6001/6007#newcode169
Line 169: __ push(r0);
I would actually write, right after restoring the context register, no pop and
instead:
if (...is_temporary()) {
  __ str(r0, MemOperand(sp));
} else {
  ASSERT(expr->location().is_nowhere());
  __ pop();
}

http://codereview.chromium.org/302002/diff/6001/6007#newcode230
Line 230: Handle<JSFunction> boilerplate = BuildBoilerplate(expr);
I'm pretty sure you'll need to check for stack overflow after calling
BuildBoilerplate, and bailout the visitor (just return) if it has.

http://codereview.chromium.org/302002/diff/6001/6007#newcode232
Line 232: // Call the runtime to instantiate the function boilerplate object.
The comments are screwy here.  This comment is describing the ASSERT?  The
comment seems unneeded.

http://codereview.chromium.org/302002/diff/6001/6007#newcode244
Line 244: ASSERT(expr->location().is_nowhere());
If a function literal falls in the forest....  Seriously, we should investigate
whether we have to do any of this in the case that the value is not needed.  (As
a separate TODO.)

http://codereview.chromium.org/302002/diff/6001/6004
File src/compiler.cc (right):

http://codereview.chromium.org/302002/diff/6001/6004#newcode501
Line 501: CHECK_BAILOUT;
For symmetry, this CHECK_BAILOUT should move after the call to Visit.

http://codereview.chromium.org/302002/diff/6001/6004#newcode725
Line 725: // In case of JS runtime function bail out.
We have to be careful here that we are not calling one of the inlined runtime
calls, because we can't yet generate them.  There is a
CodeGenerator::CHeckForInlineRuntimeCall that should work.

http://codereview.chromium.org/302002/diff/6001/6005
File src/fast-codegen.cc (right):

http://codereview.chromium.org/302002/diff/6001/6005#newcode125
Line 125: static Handle<Code> ComputeLazyCompile(int argc) {
This should move to handles.cc (best) or become a static member function of
CodeGenerator to avoid duplicating it here.

http://codereview.chromium.org/302002/diff/6001/6005#newcode145
Line 145: 
There is a missing notification to the debugger (from
CodeGenerator::BuildBoilerplate) and also setting the expected number of
properties.

We should be able to make CodeGenerator::BuildBoilerplate static (with some
extra plumbing to propagate stack overflows) and reuse that?

http://codereview.chromium.org/302002/diff/6001/6005#newcode150
Line 150: Handle<Code> FastCodeGenerator::ComputeCallInitialize(int argc,
This could be in handles.cc or static in CodeGenerator.

http://codereview.chromium.org/302002/diff/6001/6002
File src/fast-codegen.h (right):

http://codereview.chromium.org/302002/diff/6001/6002#newcode54
Line 54: void ProcessDeclarations();
Visit declarations?

http://codereview.chromium.org/302002/diff/6001/6003
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/302002/diff/6001/6003#newcode68
Line 68: Comment cmnt(masm_, "[ declarations");
Same formatting comment as in ARM.

http://codereview.chromium.org/302002/diff/6001/6003#newcode119
Line 119: static Operand ContextOperand(Register context, int index) {
Use CodeGenerator.

http://codereview.chromium.org/302002/diff/6001/6003#newcode133
Line 133: __ push(ContextOperand(esi, Context::GLOBAL_INDEX));
Use CodeGenerator::GlobalObject()

http://codereview.chromium.org/302002/diff/6001/6006
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/302002/diff/6001/6006#newcode69
Line 69: Comment cmnt(masm_, "[ declarations");
Same comment as ARM.

[fschneider, 2009-10-20 17:32:59]

Thanks. I uploaded a new patch set.

http://codereview.chromium.org/302002/diff/6001/6007
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/302002/diff/6001/6007#newcode89
Line 89: Comment cmnt(masm_, "[ declarations");
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Format this the same as the other blocks: comment on the same line as the
brace
> and capitalize Declarations.

DONE.

http://codereview.chromium.org/302002/diff/6001/6007#newcode90
Line 90: ProcessDeclarations();
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Override AstVisitor::VisitDeclarations?

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode125
Line 125: Comment cmnt(masm_, "[Block");
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Space after "[" in comment string for uniformity.

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode131
Line 131: static MemOperand ContextOperand(Register context, int index) {
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Get rid of this function and just call CodeGenerator::ContextOperand.

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode136
Line 136: void FastCodeGenerator::VisitCall(Call* expr) {
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> I like to keep these sorted in the same order as the node lists in ast.h (or
at
> least, all the expressions together and all the statements together).

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode141
Line 141: 
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Blank line not needed.

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode142
Line 142: ASSERT(var != NULL && !var->is_this() && var->is_global());
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Might add !var->is_possibly_eval() to the assert for safety.
> 
> If the assert is a conjunction, we usually write each conjunct as a separate
> assert (within reason) so it's easy to see which one failed.

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode147
Line 147: __ ldr(r0, ContextOperand(cp, Context::GLOBAL_INDEX));
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Use CodeGenerator::GlobalObject()?

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode167
Line 167: __ add(sp, sp, Operand(1 * kPointerSize));
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> __ pop() (with no argument) does the same thing and is shorter.

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode169
Line 169: __ push(r0);
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> I would actually write, right after restoring the context register, no pop and
> instead:
> if (...is_temporary()) {
>   __ str(r0, MemOperand(sp));
> } else {
>   ASSERT(expr->location().is_nowhere());
>   __ pop();
> }

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode230
Line 230: Handle<JSFunction> boilerplate = BuildBoilerplate(expr);
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> I'm pretty sure you'll need to check for stack overflow after calling
> BuildBoilerplate, and bailout the visitor (just return) if it has.

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode232
Line 232: // Call the runtime to instantiate the function boilerplate object.
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> The comments are screwy here.  This comment is describing the ASSERT?  The
> comment seems unneeded.

Done.

http://codereview.chromium.org/302002/diff/6001/6007#newcode244
Line 244: ASSERT(expr->location().is_nowhere());
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> If a function literal falls in the forest....  Seriously, we should
investigate
> whether we have to do any of this in the case that the value is not needed. 
(As
> a separate TODO.)

Agreed.

http://codereview.chromium.org/302002/diff/6001/6004
File src/compiler.cc (right):

http://codereview.chromium.org/302002/diff/6001/6004#newcode501
Line 501: CHECK_BAILOUT;
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> For symmetry, this CHECK_BAILOUT should move after the call to Visit.

Done.

http://codereview.chromium.org/302002/diff/6001/6004#newcode725
Line 725: // In case of JS runtime function bail out.
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> We have to be careful here that we are not calling one of the inlined runtime
> calls, because we can't yet generate them.  There is a
> CodeGenerator::CHeckForInlineRuntimeCall that should work.

I'm not exactly sure what to do here. I added the following check:

if (expr->name()->Get(0) == '_' && 
    CodeGenerator::FindInlineRuntimeLUT(expr->name()) != NULL) {
    BAILOUT("InlineRuntimeCall");
  }

and made CodeGenerator::FindInlineRuntimeLUT public.

http://codereview.chromium.org/302002/diff/6001/6005
File src/fast-codegen.cc (right):

http://codereview.chromium.org/302002/diff/6001/6005#newcode125
Line 125: static Handle<Code> ComputeLazyCompile(int argc) {
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> This should move to handles.cc (best) or become a static member function of
> CodeGenerator to avoid duplicating it here.

Done.

http://codereview.chromium.org/302002/diff/6001/6005#newcode145
Line 145: 
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> There is a missing notification to the debugger (from
> CodeGenerator::BuildBoilerplate) and also setting the expected number of
> properties.
> 

Done.

> We should be able to make CodeGenerator::BuildBoilerplate static (with some
> extra plumbing to propagate stack overflows) and reuse that?

I agree. It's not nice to duplicate the code here. Although the existing
codegen.cc will go away eventually, right? For now I kept
FastCodeGenerator::BuildBoilerplate.

http://codereview.chromium.org/302002/diff/6001/6005#newcode150
Line 150: Handle<Code> FastCodeGenerator::ComputeCallInitialize(int argc,
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> This could be in handles.cc or static in CodeGenerator.

Done.

http://codereview.chromium.org/302002/diff/6001/6002
File src/fast-codegen.h (right):

http://codereview.chromium.org/302002/diff/6001/6002#newcode54
Line 54: void ProcessDeclarations();
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Visit declarations?

Done.

http://codereview.chromium.org/302002/diff/6001/6003
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/302002/diff/6001/6003#newcode68
Line 68: Comment cmnt(masm_, "[ declarations");
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Same formatting comment as in ARM.

Done.

http://codereview.chromium.org/302002/diff/6001/6003#newcode119
Line 119: static Operand ContextOperand(Register context, int index) {
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Use CodeGenerator.

Done.

http://codereview.chromium.org/302002/diff/6001/6003#newcode133
Line 133: __ push(ContextOperand(esi, Context::GLOBAL_INDEX));
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Use CodeGenerator::GlobalObject()

Done.

http://codereview.chromium.org/302002/diff/6001/6006
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/302002/diff/6001/6006#newcode69
Line 69: Comment cmnt(masm_, "[ declarations");
On 2009/10/20 14:49:12, Kevin Millikin wrote:
> Same comment as ARM.

Done.

[Kevin Millikin (Chromium), 2009-10-21 07:39:28]

Style comments, but LGTM.

http://codereview.chromium.org/302002/diff/8002/9011
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/302002/diff/8002/9011#newcode202
Line 202: static InlineRuntimeLUT* FindInlineRuntimeLUT(Handle<String> name);
You could just make CodeGenSelector a friend of CodeGenerator and not have to
expose this in the public interface.

It's a little weird for a public member function to export a pointer to a
private one.

http://codereview.chromium.org/302002/diff/8002/9006
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/302002/diff/8002/9006#newcode89
Line 89: Comment cmnt(masm_, "[ Declarations");
We have a mix in our codebase of opening brace on a line by itself to start a
block and first line of code on the same line as opening brace, so either is
acceptable.

However, we probably shouldn't mix them in the same function and code should be
formatted to match the code around it, so change this one or the others.  (I
prefer the other style that saves on vertical space.  Economizing on vertical
space where appropriate is encouraged by the coding guide.)

http://codereview.chromium.org/302002/diff/8002/9004
File src/fast-codegen.cc (right):

http://codereview.chromium.org/302002/diff/8002/9004#newcode126
Line 126: static Handle<Code> ComputeLazyCompile(int argc) {
Can you get rid of this one?

http://codereview.chromium.org/302002/diff/8002/9004#newcode138
Line 138: Handle<Code> code = ComputeLazyCompile(fun->num_parameters());
Can you call CodeGenerator::ComputeLazyCompile here?

[fschneider, 2009-10-21 09:08:30]

Uploaded new patch set.

http://codereview.chromium.org/302002/diff/8002/9011
File src/arm/codegen-arm.h (right):

http://codereview.chromium.org/302002/diff/8002/9011#newcode202
Line 202: static InlineRuntimeLUT* FindInlineRuntimeLUT(Handle<String> name);
On 2009/10/21 07:39:28, Kevin Millikin wrote:
> You could just make CodeGenSelector a friend of CodeGenerator and not have to
> expose this in the public interface.
> 
> It's a little weird for a public member function to export a pointer to a
> private one.

Done.

http://codereview.chromium.org/302002/diff/8002/9006
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/302002/diff/8002/9006#newcode89
Line 89: Comment cmnt(masm_, "[ Declarations");
On 2009/10/21 07:39:28, Kevin Millikin wrote:
> We have a mix in our codebase of opening brace on a line by itself to start a
> block and first line of code on the same line as opening brace, so either is
> acceptable.
> 
> However, we probably shouldn't mix them in the same function and code should
be
> formatted to match the code around it, so change this one or the others.  (I
> prefer the other style that saves on vertical space.  Economizing on vertical
> space where appropriate is encouraged by the coding guide.)

Done. 

I also changed the order in which we generate the stack check, call
VisitDeclarations and call VisitStatements to be consistent across all
platforms.

http://codereview.chromium.org/302002/diff/8002/9004
File src/fast-codegen.cc (right):

http://codereview.chromium.org/302002/diff/8002/9004#newcode126
Line 126: static Handle<Code> ComputeLazyCompile(int argc) {
On 2009/10/21 07:39:28, Kevin Millikin wrote:
> Can you get rid of this one?

Done.

http://codereview.chromium.org/302002/diff/8002/9004#newcode138
Line 138: Handle<Code> code = ComputeLazyCompile(fun->num_parameters());
On 2009/10/21 07:39:28, Kevin Millikin wrote:
> Can you call CodeGenerator::ComputeLazyCompile here?

Done.