Add an assigned variables analysis....

Add an assigned variables analysis.

This change adds a pass over the AST that computes the
set of assigned variables for locals and parameters for each expression.

The result of this analysis is used to for two purposes:
1. Recognize variables that are trivial subexpressions. A left sub-expression
   of a binary operation is trivial if it is a local variable or a parameter
   and it is not assigned in the right sub-expression. In the case of a 
   trivial left sub-expression we evaluate the right first.
   Currently only binary operations and compare operations are considered
   when finding trivial left sub-expressions.

2. Recogize certain simple for-loops with a constant trip count where the loop
   variable is always within smi range. If the loop count variable is not
   assigned in the body of the loop (except in the update expression the
   for-loop). This allows omitting smi checks on operation using the loop
   count variable.


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

[fschneider, 2010-03-05 14:06:34]

Ready for first round of review.

I tried to reduce the number of bit-vector copies by accumulating results over
statements and only computing the proper set of assigned variables for
expressions. However, it seems there is still a significant cost in compile
time.

[Kevin Millikin (Chromium), 2010-03-08 10:55:32]

First round of comments on the assigned variables visitor.

I think you can simply accumulate assigned vars in expressions if you do it
right-to-left.  That should eliminate some of the clearing and fresh bit vector
construction.

http://codereview.chromium.org/669155/diff/32/1042
File src/ast.cc (right):

http://codereview.chromium.org/669155/diff/32/1042#newcode81
src/ast.cc:81: bool VariableProxy::IsLocal() {
This seems misleading.  I think of a variable "IsLocal" if it is in the function
body scope.  This predicate tests is local and is stack allocated.

http://codereview.chromium.org/669155/diff/32/1039
File src/ast.h (right):

http://codereview.chromium.org/669155/diff/32/1039#newcode1029
src/ast.h:1029: bool IsParam();
IsParameter.

http://codereview.chromium.org/669155/diff/32/1032
File src/data-flow.cc (right):

http://codereview.chromium.org/669155/diff/32/1032#newcode584
src/data-flow.cc:584: if (stmt->init()->AsExpressionStatement() != NULL &&
Since this is a syntactic property, I think it's fair to make it a virtual
function on AstNode.  That way, you avoid all the type testing here.

Assignment* init = stmt->init()->AsSimpleAssignment();

where

Assignment* ExpressionStatement::AsSimpleAssignent() {
  return expression()->AsSimpleAssignment();
}

Assignment* Block::AsSimpleAssignment() {
  if (statements_.length() != 1) return NULL;
  return statements_[0]->AsSimpleAssignment();
}

Assignment* Assignment::AsSimpleAssignment() {
  return this;
}

http://codereview.chromium.org/669155/diff/32/1032#newcode601
src/data-flow.cc:601: if (init->target()->AsVariableProxy() == NULL) return
NULL;
I'm concerned that you don't seem to check the operator in the assignment
expression.  What if it's += for instance?

This reads better if you write:

VariableProxy* proxy = init->target()->AsVariableProxy();
if (proxy == NULL || !proxy->IsLocal()) return NULL;

http://codereview.chromium.org/669155/diff/32/1032#newcode609
src/data-flow.cc:609: if (init->value()->AsLiteral() == NULL ||
Likewise:

Literal* value = init->value()->AsLiteral();
if (value == NULL || !value->handle()->IsSmi()) return NULL;
int init_value = Smi::cast(*value->handle())->value();

http://codereview.chromium.org/669155/diff/32/1032#newcode650
src/data-flow.cc:650: if (init_value > term_value && update->op() != Token::DEC)
return NULL;
Does this handle all the possibilities?  What about 'for (var i = 0; i > 0;
i++)'?

http://codereview.chromium.org/669155/diff/32/1032#newcode657
src/data-flow.cc:657: int  AssignedVariablesAnalyzer::BitIndex(Variable* var) {
Extra space after 'int'.  Maybe it's premature, but it seems like this function
should be inlined.

http://codereview.chromium.org/669155/diff/32/1032#newcode683
src/data-flow.cc:683: void
AssignedVariablesAnalyzer::VisitStatements(ZoneList<Statement*>* stmts) {
This is the default implementation, so it doesn't need to be overridden.

http://codereview.chromium.org/669155/diff/32/1032#newcode698
src/data-flow.cc:698: result.CopyFrom(av_);
This idiom comes up repeatedly: create fresh bit vector, initialize it, and then
clear the original.  There must be some way to combine them.

http://codereview.chromium.org/669155/diff/32/1032#newcode714
src/data-flow.cc:714: Visit(stmt->condition());
I expect the call to Union right after this, because it seems like that's where
you switch from back from collecting to accumulating the assigned vars.

http://codereview.chromium.org/669155/diff/32/1032#newcode716
src/data-flow.cc:716: if (stmt->HasElseStatement()) {
It seems simpler just to visit the (possibly empty) else statement.

http://codereview.chromium.org/669155/diff/32/1032#newcode782
src/data-flow.cc:782: Visit(stmt->cond());
Union after this Visit?

http://codereview.chromium.org/669155/diff/32/1032#newcode792
src/data-flow.cc:792: Visit(stmt->cond());
And here?

http://codereview.chromium.org/669155/diff/32/1032#newcode824
src/data-flow.cc:824: Variable* var = FindSmiLoopVariable(stmt);
This needs a comment that av_ is the assigned variables in the body.

http://codereview.chromium.org/669155/diff/32/1041
File src/data-flow.h (right):

http://codereview.chromium.org/669155/diff/32/1041#newcode187
src/data-flow.h:187: class AssignedVariablesAnalyzer : public AstVisitor {
Needs some simple comment about what it's doing.  E.g., collect assigned
variables in expressions right-to-left and annotate variables and for loops....

http://codereview.chromium.org/669155/diff/32/1041#newcode189
src/data-flow.h:189: explicit AssignedVariablesAnalyzer(FunctionLiteral* fun);
I like it better if fun is an argument to Analyze, not the constructor.

http://codereview.chromium.org/669155/diff/32/1041#newcode194
src/data-flow.h:194: int  BitIndex(Variable* var);
Extra space after int.

http://codereview.chromium.org/669155/diff/32/1041#newcode206
src/data-flow.h:206: int num_params_;
If you've got the function literal, you don't need num_params_ or num_locals_
here---you can just get them from the literal or scope.  They only seem to be
used once or twice anyway.

Most uses of num_vars_ can be replaced with av_.length().

[fschneider, 2010-03-08 17:26:47]

Uploaded new version with comments addressed.

I'm still processing expressions left-to-right (and saving the intermediate
result of the left). Are we sure that we never need the set of assigned
variables of a left sub-tree?
If we need this information it does not make sense to change the traversal order
now and change it back later.

http://codereview.chromium.org/669155/diff/32/1042
File src/ast.cc (right):

http://codereview.chromium.org/669155/diff/32/1042#newcode81
src/ast.cc:81: bool VariableProxy::IsLocal() {
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> This seems misleading.  I think of a variable "IsLocal" if it is in the
function
> body scope.  This predicate tests is local and is stack allocated.

Done.

http://codereview.chromium.org/669155/diff/32/1039
File src/ast.h (right):

http://codereview.chromium.org/669155/diff/32/1039#newcode1029
src/ast.h:1029: bool IsParam();
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> IsParameter.

Done.

http://codereview.chromium.org/669155/diff/32/1032
File src/data-flow.cc (right):

http://codereview.chromium.org/669155/diff/32/1032#newcode584
src/data-flow.cc:584: if (stmt->init()->AsExpressionStatement() != NULL &&
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> Since this is a syntactic property, I think it's fair to make it a virtual
> function on AstNode.  That way, you avoid all the type testing here.
> 
> Assignment* init = stmt->init()->AsSimpleAssignment();
> 
> where
> 
> Assignment* ExpressionStatement::AsSimpleAssignent() {
>   return expression()->AsSimpleAssignment();
> }
> 
> Assignment* Block::AsSimpleAssignment() {
>   if (statements_.length() != 1) return NULL;
>   return statements_[0]->AsSimpleAssignment();
> }
> 
> Assignment* Assignment::AsSimpleAssignment() {
>   return this;
> }

Done. Good idea.

http://codereview.chromium.org/669155/diff/32/1032#newcode601
src/data-flow.cc:601: if (init->target()->AsVariableProxy() == NULL) return
NULL;
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> I'm concerned that you don't seem to check the operator in the assignment
> expression.  What if it's += for instance?
> 
> This reads better if you write:
> 
> VariableProxy* proxy = init->target()->AsVariableProxy();
> if (proxy == NULL || !proxy->IsLocal()) return NULL;
> 

Done.

http://codereview.chromium.org/669155/diff/32/1032#newcode609
src/data-flow.cc:609: if (init->value()->AsLiteral() == NULL ||
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> Likewise:
> 
> Literal* value = init->value()->AsLiteral();
> if (value == NULL || !value->handle()->IsSmi()) return NULL;
> int init_value = Smi::cast(*value->handle())->value();

Done.

http://codereview.chromium.org/669155/diff/32/1032#newcode650
src/data-flow.cc:650: if (init_value > term_value && update->op() != Token::DEC)
return NULL;
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> Does this handle all the possibilities?  What about 'for (var i = 0; i > 0;
> i++)'?

Right. I need to check for the case where bound1 == bound2. 
For now I think I'll accept missing the cases where the loop is never entered or
executed once because those should not hurt. Otherwise the condition would be
quite complicated.

http://codereview.chromium.org/669155/diff/32/1032#newcode657
src/data-flow.cc:657: int  AssignedVariablesAnalyzer::BitIndex(Variable* var) {
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> Extra space after 'int'.  Maybe it's premature, but it seems like this
function
> should be inlined.

Done.

http://codereview.chromium.org/669155/diff/32/1032#newcode683
src/data-flow.cc:683: void
AssignedVariablesAnalyzer::VisitStatements(ZoneList<Statement*>* stmts) {
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> This is the default implementation, so it doesn't need to be overridden.

Done.

http://codereview.chromium.org/669155/diff/32/1032#newcode714
src/data-flow.cc:714: Visit(stmt->condition());
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> I expect the call to Union right after this, because it seems like that's
where
> you switch from back from collecting to accumulating the assigned vars.

I could write:

result.Union(av_);
av_.Clear();

here but it seems redundant since I don't need to the set of assigned variables
of the then- and the else-block separately. So I only do union/clear before
visiting expressions (where I also assert that av_ is empty) and in for-loops
where I need the result of from the loop body.

http://codereview.chromium.org/669155/diff/32/1032#newcode714
src/data-flow.cc:714: Visit(stmt->condition());
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> I expect the call to Union right after this, because it seems like that's
where
> you switch from back from collecting to accumulating the assigned vars.

Agree. I'll combine the construction+copying into a copy constructor for now.

http://codereview.chromium.org/669155/diff/32/1032#newcode716
src/data-flow.cc:716: if (stmt->HasElseStatement()) {
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> It seems simpler just to visit the (possibly empty) else statement.

Done.

http://codereview.chromium.org/669155/diff/32/1032#newcode782
src/data-flow.cc:782: Visit(stmt->cond());
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> Union after this Visit?

As above no need before visiting statements.

http://codereview.chromium.org/669155/diff/32/1032#newcode792
src/data-flow.cc:792: Visit(stmt->cond());
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> And here?

As above no need before visiting statements.

http://codereview.chromium.org/669155/diff/32/1032#newcode824
src/data-flow.cc:824: Variable* var = FindSmiLoopVariable(stmt);
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> This needs a comment that av_ is the assigned variables in the body.

Done.

http://codereview.chromium.org/669155/diff/32/1041
File src/data-flow.h (right):

http://codereview.chromium.org/669155/diff/32/1041#newcode187
src/data-flow.h:187: class AssignedVariablesAnalyzer : public AstVisitor {
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> Needs some simple comment about what it's doing.  E.g., collect assigned
> variables in expressions right-to-left and annotate variables and for
loops....

Done.

http://codereview.chromium.org/669155/diff/32/1041#newcode189
src/data-flow.h:189: explicit AssignedVariablesAnalyzer(FunctionLiteral* fun);
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> I like it better if fun is an argument to Analyze, not the constructor.

Since the member bitvector av_ is an embedded instance it needs to be
initialized in the constructor with the correct length. I'm not sure if it is
worth having a pointer to a bitvector instead. It would add another indirection
on accessing the bitvector, wouldn't it?

http://codereview.chromium.org/669155/diff/32/1041#newcode194
src/data-flow.h:194: int  BitIndex(Variable* var);
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> Extra space after int.

Done.

http://codereview.chromium.org/669155/diff/32/1041#newcode206
src/data-flow.h:206: int num_params_;
On 2010/03/08 10:55:32, Kevin Millikin wrote:
> If you've got the function literal, you don't need num_params_ or num_locals_
> here---you can just get them from the literal or scope.  They only seem to be
> used once or twice anyway.
> 
> Most uses of num_vars_ can be replaced with av_.length().

Done.

[Kevin Millikin (Chromium), 2010-03-09 06:03:47]

http://codereview.chromium.org/669155/diff/32/1032
File src/data-flow.cc (right):

http://codereview.chromium.org/669155/diff/32/1032#newcode714
src/data-flow.cc:714: Visit(stmt->condition());
On 2010/03/08 17:26:47, fschneider wrote:
> I could write:
> 
> result.Union(av_);
> av_.Clear();

I don't think you'd write that.  You mean:

av_.Union(result)

My comment is that you *should* write that.  In fact you might even have

void ProcessExpression(Expression* expr) {
  BitVector saved(num_vars);
  saved.CopyFrom(av_);
  av_.Clear();
  Visit(expr);
  av_.Union(saved);
}

void VisitIfStatement(IfStatement stmt) {
  ProcessExpression(stmt->condition());
  Visit(stmt->then_statement());
  Visit(stmt->else_statement());
}

> So I only do union/clear before
> visiting expressions (where I also assert that av_ is empty) and in for-loops
> where I need the result of from the loop body.

My suggestion was not to do it more than was needed.  My suggestion was to do it
*exactly* where needed.

http://codereview.chromium.org/669155/diff/32/1032#newcode782
src/data-flow.cc:782: Visit(stmt->cond());
On 2010/03/08 17:26:47, fschneider wrote:
> On 2010/03/08 10:55:32, Kevin Millikin wrote:
> > Union after this Visit?
> 
> As above no need before visiting statements.

There is also no need to have the accumulator reset for the entire loop.

http://codereview.chromium.org/669155/diff/32/1032#newcode801
src/data-flow.cc:801: av_.Clear();
Here is another place where the implementation seems too complicated.

void VisitForStatement(ForStatement* stmt) {
  BitVector saved(num_vars_);
  if (stmt->init() != NULL) Visit(stmt->init());
  if (stmt->cond() != NULL) {
    // ProcessExpression
    saved.CopyFrom(av_);
    av_.Clear();
    Visit(stmt->cond());
    av_.Union(saved);
  }
  if (stmt->next() != NULL) Visit(stmt->next());
  saved.CopyFrom(av_);
  av_.Clear();
  Visit(stmt->body());
  ....

[fschneider, 2010-03-10 11:52:23]

http://codereview.chromium.org/669155/diff/32/1032
File src/data-flow.cc (right):

http://codereview.chromium.org/669155/diff/32/1032#newcode714
src/data-flow.cc:714: Visit(stmt->condition());
On 2010/03/09 06:03:47, Kevin Millikin wrote:
> On 2010/03/08 17:26:47, fschneider wrote:
> > I could write:
> > 
> > result.Union(av_);
> > av_.Clear();
> 
> I don't think you'd write that.  You mean:
> 
> av_.Union(result)
> 
> My comment is that you *should* write that.  In fact you might even have
> 
> void ProcessExpression(Expression* expr) {
>   BitVector saved(num_vars);
>   saved.CopyFrom(av_);
>   av_.Clear();
>   Visit(expr);
>   av_.Union(saved);
> }
> 
> void VisitIfStatement(IfStatement stmt) {
>   ProcessExpression(stmt->condition());
>   Visit(stmt->then_statement());
>   Visit(stmt->else_statement());
> }
> 
> > So I only do union/clear before
> > visiting expressions (where I also assert that av_ is empty) and in
for-loops
> > where I need the result of from the loop body.
> 
> My suggestion was not to do it more than was needed.  My suggestion was to do
it
> *exactly* where needed.
> 

Oh ok. That was a misunderstanding on my side. You meant moving

av_.Union(result) 

and not inserting a new one.

But ProcessExpression is definitely a good idea. The only places where this
abstraction does not fit in are ast nodes that contains two or more
sub-expressions (e.g. for binary operation:

ProcessExpression(left);
ProcessExpression(right);

would too many copies. Instead I do 

Visit(left);
ProcessExpression(right);

Also for call arguments, array literals, etc. I'll explicitly allocate a
temporary result bit-vector for those nodes to avoid excessive copying.

http://codereview.chromium.org/669155/diff/32/1032#newcode782
src/data-flow.cc:782: Visit(stmt->cond());
On 2010/03/09 06:03:47, Kevin Millikin wrote:
> On 2010/03/08 17:26:47, fschneider wrote:
> > On 2010/03/08 10:55:32, Kevin Millikin wrote:
> > > Union after this Visit?
> > 
> > As above no need before visiting statements.
> 
> There is also no need to have the accumulator reset for the entire loop.

Done. I'm calling ProcessExpression for the condition and Visit for the body.

http://codereview.chromium.org/669155/diff/32/1032#newcode801
src/data-flow.cc:801: av_.Clear();
On 2010/03/09 06:03:47, Kevin Millikin wrote:
> Here is another place where the implementation seems too complicated.
> 
> void VisitForStatement(ForStatement* stmt) {
>   BitVector saved(num_vars_);
>   if (stmt->init() != NULL) Visit(stmt->init());
>   if (stmt->cond() != NULL) {
>     // ProcessExpression
>     saved.CopyFrom(av_);
>     av_.Clear();
>     Visit(stmt->cond());
>     av_.Union(saved);
>   }
>   if (stmt->next() != NULL) Visit(stmt->next());
>   saved.CopyFrom(av_);
>   av_.Clear();
>   Visit(stmt->body());
>   ....

Done. Also using ProcessExpression here.

[Kevin Millikin (Chromium), 2010-03-10 14:37:06]

http://codereview.chromium.org/669155/diff/25001/11009
File src/ast.h (right):

http://codereview.chromium.org/669155/diff/25001/11009#newcode128
src/ast.h:128: virtual Block* AsBlock() { return NULL; }
Is this used?

http://codereview.chromium.org/669155/diff/25001/11009#newcode144
src/ast.h:144: virtual Assignment* AsSimpleAssignment() { return NULL; }
Maybe this should be a function on Statement instead of AstNode, so it's clearly
not AsAssignment().

http://codereview.chromium.org/669155/diff/25001/11009#newcode339
src/ast.h:339: bool IsSimpleInitializerBlock();
Is this needed?

http://codereview.chromium.org/669155/diff/25001/11009#newcode531
src/ast.h:531: virtual CountOperation* AsCountOperation() {
Maybe this should be something like Statement::StatementAsCountOperation.

http://codereview.chromium.org/669155/diff/25001/11002
File src/data-flow.cc (right):

http://codereview.chromium.org/669155/diff/25001/11002#newcode1123
src/data-flow.cc:1123: VariableProxy* proxy = init->target()->AsVariableProxy();
I think this function should be rewritten to use Variable* instead of
VariableProxy* where possible.  It seems more direct that way.

http://codereview.chromium.org/669155/diff/25001/11002#newcode1134
src/data-flow.cc:1134: // The condition must be a comparison.
This reads a bit better if you write:

CompareOperation* cond = stmt->cond()->AsCompareOperation();
if (cond == NULL) return NULL;
if (cond->op() ...) ...

http://codereview.chromium.org/669155/diff/25001/11002#newcode1153
src/data-flow.cc:1153: // The update expression must be a count operation.
This reads a bit better if you write:

CountOperation* update = stmt->next()->AsCountOperation();
if (update == NULL) return NULL;
...

http://codereview.chromium.org/669155/diff/25001/11002#newcode1193
src/data-flow.cc:1193: if (var != NULL &&
if (var != NULL && var->IsStackAllocated()) ...

http://codereview.chromium.org/669155/diff/25001/11002#newcode1262
src/data-flow.cc:1262: BitVector result(av_);
Maybe call this something other than result, since it's not the result that's
actually returned.  "accumulator"?

http://codereview.chromium.org/669155/diff/25001/11002#newcode1294
src/data-flow.cc:1294: if (stmt->cond() != NULL) {
Why does this one have braces?  Too long for one line?

http://codereview.chromium.org/669155/diff/25001/11002#newcode1302
src/data-flow.cc:1302: BitVector result(av_);
This one isn't the result that's returned either.  Maybe "saved"?

http://codereview.chromium.org/669155/diff/25001/11002#newcode1360
src/data-flow.cc:1360: BitVector result(av_);
accumulator?

http://codereview.chromium.org/669155/diff/25001/11011
File src/data-flow.h (right):

http://codereview.chromium.org/669155/diff/25001/11011#newcode509
src/data-flow.h:509: 
It looks like an extra blank line after private:.

[fschneider, 2010-03-10 15:47:48]

Thanks. I addressed the comments.

I also added marking trivial receivers of property stores in the analysis, but
do not make use of it yet in the code generator.

http://codereview.chromium.org/669155/diff/25001/11009
File src/ast.h (right):

http://codereview.chromium.org/669155/diff/25001/11009#newcode128
src/ast.h:128: virtual Block* AsBlock() { return NULL; }
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> Is this used?

No. Removed.

http://codereview.chromium.org/669155/diff/25001/11009#newcode144
src/ast.h:144: virtual Assignment* AsSimpleAssignment() { return NULL; }
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> Maybe this should be a function on Statement instead of AstNode, so it's
clearly
> not AsAssignment().

Done. As discussed now a Statement function.

http://codereview.chromium.org/669155/diff/25001/11009#newcode339
src/ast.h:339: bool IsSimpleInitializerBlock();
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> Is this needed?

No. Removed.

http://codereview.chromium.org/669155/diff/25001/11009#newcode531
src/ast.h:531: virtual CountOperation* AsCountOperation() {
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> Maybe this should be something like Statement::StatementAsCountOperation.

Done.

http://codereview.chromium.org/669155/diff/25001/11002
File src/data-flow.cc (right):

http://codereview.chromium.org/669155/diff/25001/11002#newcode1123
src/data-flow.cc:1123: VariableProxy* proxy = init->target()->AsVariableProxy();
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> I think this function should be rewritten to use Variable* instead of
> VariableProxy* where possible.  It seems more direct that way.

Done. (also for the other parts dealing with the loop variable in this function)

http://codereview.chromium.org/669155/diff/25001/11002#newcode1134
src/data-flow.cc:1134: // The condition must be a comparison.
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> This reads a bit better if you write:
> 
> CompareOperation* cond = stmt->cond()->AsCompareOperation();
> if (cond == NULL) return NULL;
> if (cond->op() ...) ...

Done.

http://codereview.chromium.org/669155/diff/25001/11002#newcode1153
src/data-flow.cc:1153: // The update expression must be a count operation.
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> This reads a bit better if you write:
> 
> CountOperation* update = stmt->next()->AsCountOperation();
> if (update == NULL) return NULL;
> ...

Done.

http://codereview.chromium.org/669155/diff/25001/11002#newcode1193
src/data-flow.cc:1193: if (var != NULL &&
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> if (var != NULL && var->IsStackAllocated()) ...

Done. Also in some other places.

http://codereview.chromium.org/669155/diff/25001/11002#newcode1262
src/data-flow.cc:1262: BitVector result(av_);
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> Maybe call this something other than result, since it's not the result that's
> actually returned.  "accumulator"?

Agree. It is used differently for different ast node types. I changed it to
either "result" where I use to as accumulator for the final result of a node or
"saved_av" where just save the state before call to visit.

http://codereview.chromium.org/669155/diff/25001/11002#newcode1294
src/data-flow.cc:1294: if (stmt->cond() != NULL) {
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> Why does this one have braces?  Too long for one line?

Done.

http://codereview.chromium.org/669155/diff/25001/11002#newcode1302
src/data-flow.cc:1302: BitVector result(av_);
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> This one isn't the result that's returned either.  Maybe "saved"?

Done.

http://codereview.chromium.org/669155/diff/25001/11002#newcode1360
src/data-flow.cc:1360: BitVector result(av_);
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> accumulator?

Done.

http://codereview.chromium.org/669155/diff/25001/11011
File src/data-flow.h (right):

http://codereview.chromium.org/669155/diff/25001/11011#newcode509
src/data-flow.h:509: 
On 2010/03/10 14:37:06, Kevin Millikin wrote:
> It looks like an extra blank line after private:.

Done.

[Kevin Millikin (Chromium), 2010-03-10 16:41:14]

I think you should remove Scope::num_stack_variables.

Other than that, there are some suggestions and LGTM.

http://codereview.chromium.org/669155/diff/35001/9010
File src/compiler.cc (right):

http://codereview.chromium.org/669155/diff/35001/9010#newcode82
src/compiler.cc:82: if (function->scope()->num_stack_variables() > 0) {
Could we get rid of num_stack_variables and use

if (function->scope()->num_parameters() > 0 ||
    function->scope()->num_stack_slots() > 0) { ... }

http://codereview.chromium.org/669155/diff/35001/9002
File src/data-flow.cc (right):

http://codereview.chromium.org/669155/diff/35001/9002#newcode1178
src/data-flow.cc:1178: void
AssignedVariablesAnalyzer::RecordAssignedVar(Variable* var) {
It might make more sense to require var to be non-NULL and check for it at the
call sites.

http://codereview.chromium.org/669155/diff/35001/9002#newcode1185
src/data-flow.cc:1185: void
AssignedVariablesAnalyzer::ComputeTrivialExpression(Expression* expr) {
It doesn't really Compute.  Maybe "MarkIfTrivial"?

http://codereview.chromium.org/669155/diff/35001/9002#newcode1189
src/data-flow.cc:1189: !var->is_this() &&
You could remove the special case for this in VariableProxy::IsTrivial, and have

if (var != NULL &&
    var->IsStackAllocated() &&
    (var->is_this() || !av_.Contains(BitIndex(var)))) {
  ....
}

http://codereview.chromium.org/669155/diff/35001/9002#newcode1423
src/data-flow.cc:1423: RecordAssignedVar(var);
Then you'd have

if (var != NULL) RecordAssignedVar(var);

here.

http://codereview.chromium.org/669155/diff/35001/9002#newcode1425
src/data-flow.cc:1425: ProcessExpression(expr->value());
It doesn't make any difference, but since the assignment to var happens after
the evaluation of expr->value(), it might make sense to have

ProcessExpression(expr->value());
RecordAssignedVar(var)...

http://codereview.chromium.org/669155/diff/35001/9006
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/669155/diff/35001/9006#newcode1174
src/ia32/virtual-frame-ia32.cc:1174: if (proxy != NULL) {
I don't think there needs to be a special case for parameter -1:

if (proxy != NULL) {
  Slot* slot = proxy->var()->slot();
  ASSERT(slot != NULL);
  if (slot->type() == Slot::LOCAL) {
    PushLocalAt(slot->index());
    return;
  }
  if (slot->type() == Slot::PARAMETER) {
    PushParameterAt(slot->index());
    return;
  }

http://codereview.chromium.org/669155/diff/35001/9008
File src/ia32/virtual-frame-ia32.h (right):

http://codereview.chromium.org/669155/diff/35001/9008#newcode426
src/ia32/virtual-frame-ia32.h:426: inline void SetTypeLocalAt(int index,
NumberInfo info);
Maybe SetTypeForLocalAt?

[fschneider, 2010-03-10 17:15:24]

http://codereview.chromium.org/669155/diff/35001/9010
File src/compiler.cc (right):

http://codereview.chromium.org/669155/diff/35001/9010#newcode82
src/compiler.cc:82: if (function->scope()->num_stack_variables() > 0) {
On 2010/03/10 16:41:14, Kevin Millikin wrote:
> Could we get rid of num_stack_variables and use
> 
> if (function->scope()->num_parameters() > 0 ||
>     function->scope()->num_stack_slots() > 0) { ... }

Done.

http://codereview.chromium.org/669155/diff/35001/9002
File src/data-flow.cc (right):

http://codereview.chromium.org/669155/diff/35001/9002#newcode1178
src/data-flow.cc:1178: void
AssignedVariablesAnalyzer::RecordAssignedVar(Variable* var) {
On 2010/03/10 16:41:14, Kevin Millikin wrote:
> It might make more sense to require var to be non-NULL and check for it at the
> call sites.

Done.

http://codereview.chromium.org/669155/diff/35001/9002#newcode1185
src/data-flow.cc:1185: void
AssignedVariablesAnalyzer::ComputeTrivialExpression(Expression* expr) {
On 2010/03/10 16:41:14, Kevin Millikin wrote:
> It doesn't really Compute.  Maybe "MarkIfTrivial"?

Done.

http://codereview.chromium.org/669155/diff/35001/9002#newcode1189
src/data-flow.cc:1189: !var->is_this() &&
On 2010/03/10 16:41:14, Kevin Millikin wrote:
> You could remove the special case for this in VariableProxy::IsTrivial, and
have
> 
> if (var != NULL &&
>     var->IsStackAllocated() &&
>     (var->is_this() || !av_.Contains(BitIndex(var)))) {
>   ....
> }

Done.

http://codereview.chromium.org/669155/diff/35001/9002#newcode1423
src/data-flow.cc:1423: RecordAssignedVar(var);
On 2010/03/10 16:41:14, Kevin Millikin wrote:
> Then you'd have
> 
> if (var != NULL) RecordAssignedVar(var);
> 
> here.

Done.

http://codereview.chromium.org/669155/diff/35001/9002#newcode1425
src/data-flow.cc:1425: ProcessExpression(expr->value());
On 2010/03/10 16:41:14, Kevin Millikin wrote:
> It doesn't make any difference, but since the assignment to var happens after
> the evaluation of expr->value(), it might make sense to have
> 
> ProcessExpression(expr->value());
> RecordAssignedVar(var)...

Done.

http://codereview.chromium.org/669155/diff/35001/9006
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/669155/diff/35001/9006#newcode1174
src/ia32/virtual-frame-ia32.cc:1174: if (proxy != NULL) {
On 2010/03/10 16:41:14, Kevin Millikin wrote:
> I don't think there needs to be a special case for parameter -1:
> 
> if (proxy != NULL) {
>   Slot* slot = proxy->var()->slot();
>   ASSERT(slot != NULL);
>   if (slot->type() == Slot::LOCAL) {
>     PushLocalAt(slot->index());
>     return;
>   }
>   if (slot->type() == Slot::PARAMETER) {
>     PushParameterAt(slot->index());
>     return;
>   }

Done.

http://codereview.chromium.org/669155/diff/35001/9008
File src/ia32/virtual-frame-ia32.h (right):

http://codereview.chromium.org/669155/diff/35001/9008#newcode426
src/ia32/virtual-frame-ia32.h:426: inline void SetTypeLocalAt(int index,
NumberInfo info);
On 2010/03/10 16:41:14, Kevin Millikin wrote:
> Maybe SetTypeForLocalAt?

Done.