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().
http://codereview.chromium.org/669155
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
