Initial comments. Overall looks about right.
http://codereview.chromium.org/1132005/diff/1/8 File src/ast.cc (right): http://codereview.chromium.org/1132005/diff/1/8#newcode523 src/ast.cc:523: bool VariableProxy::IsPrimitive() { return is_primitive_; } Move this function after the "not primitive" section and add a comment to the effect that variable references are primitive unless the primitivity (?) analysis determines they aren't. Also, you may want to consider parameters rewritten as explicit arguments accesses. They are aliased by the arguments object, so the safe thing is to conclude not primitive. http://codereview.chromium.org/1132005/diff/1/4 File src/data-flow.cc (right): http://codereview.chromium.org/1132005/diff/1/4#newcode2009 src/data-flow.cc:2009: while (changed) { This is a case where it might be clearer to have a do...while loop? http://codereview.chromium.org/1132005/diff/1/4#newcode2016 src/data-flow.cc:2016: for (int i = 0; i < postorder_->length(); i++) { I wonder if we converge in one fewer iteration if we iterate this list in reverse? http://codereview.chromium.org/1132005/diff/1/4#newcode2018 src/data-flow.cc:2018: if (node->IsBlockNode()) { Since we control the Node class and don't have external clients, we could just add a virtual AnalyzeTypes function that performs the body of this if for BlockNodes and nothing for the other node types. Advantage is that we don't have to expose as much of the structure of the block nodes (eg, instructions() array). Disadvantage is that it's not inlined at the call site. http://codereview.chromium.org/1132005/diff/1/7 File src/data-flow.h (right): http://codereview.chromium.org/1132005/diff/1/7#newcode217 src/data-flow.h:217: virtual BlockNode* AsBlockNode() { return NULL; } There are two kinds of type testing/casting used in V8: separate virtual IsXXX predicate and static XXX:cast (a la objects.h) and unified virtual AsXXX (a la ast.h). I prefer the former and was using it here. You could convince me to use the latter instead, but I don't think there's any reason to mix them. http://codereview.chromium.org/1132005/diff/1/7#newcode645 src/data-flow.h:645: bool IsPrimitiveDef(int i); Small comment needed for this function, especially since the name "i" is so generic. http://codereview.chromium.org/1132005 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev To unsubscribe from this group, send email to v8-dev+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
