http://codereview.chromium.org/660372/diff/1/2 File src/ast.h (right):
http://codereview.chromium.org/660372/diff/1/2#newcode239 src/ast.h:239: bitfields_ |= SideEffectFreeField::encode(is_side_effect_free); On 2010/03/02 12:43:57, Lasse Reichstein wrote:
Why update the field twice, instead of just bitfields_ = (bitfields_ & mask) | bits (ditto for the other two setter functions)
Because both versions generate the same optimized code: f7: 8d 2c 00 lea (%eax,%eax,1),%ebp fa: 89 d1 mov %edx,%ecx fc: 83 e1 c1 and $0xffffffc1,%ecx ff: 09 e9 or %ebp,%ecx 101: 89 4f 04 mov %ecx,0x4(%edi) and the first version is easier to read and understand. http://codereview.chromium.org/660372/diff/1/3 File src/rewriter.cc (right): http://codereview.chromium.org/660372/diff/1/3#newcode229 src/rewriter.cc:229: void AstOptimizer::VisitSlot(Slot* node) { On 2010/03/02 12:43:57, Lasse Reichstein wrote:
Do we ever visit the slots? If I understand it correctly, a Slot node
only
occurs as part of a variable in a variable proxy, and the
VisitVariableProxy
below doesn't appear to recurse on the variable/slot (which is good,
because the
variable and slot are shared). Maybe move this to the VisitVariableProxy slot case.
Yes, we don't need this on the slot. Removed. http://codereview.chromium.org/660372/diff/1/3#newcode233 src/rewriter.cc:233: node->type() == Slot::PARAMETER) { On 2010/03/02 12:43:57, Lasse Reichstein wrote:
Could we do contexts as well? I.e., is reading a context guaranteed to
be
side-effect free?
That can be a later addition. I don't think it is crucial for performance right now. http://codereview.chromium.org/660372/diff/1/3#newcode438 src/rewriter.cc:438: node->set_stack_height(node->expression()->stack_height()); On 2010/03/02 12:43:57, Lasse Reichstein wrote:
How about a combined setter that sets all three values in one go? It
would also
only need to update bitflags_ once.
I'm not too worried about optimizing this. It is easier to maintain this way. I think it would be premature optimization. http://codereview.chromium.org/660372/diff/1/3#newcode538 src/rewriter.cc:538: node->left()->expression_size() + 1); On 2010/03/02 12:43:57, Lasse Reichstein wrote:
Second "left()" -> "right()".
Done. http://codereview.chromium.org/660372 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
