Add syntax checker for side-effect-free expressions to AstOptimizer in rewrit...

Add syntax checker for side-effect-free expressions to AstOptimizer in rewriter.cc.  Add bit fields for syntax checker results to Expression in ast.h.

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

[William Hesse, 2010-03-02 12:14:08]

Adds a syntax checker for the expressions supported by the side-effect-free
expression compiler to AstOptimizer.  Also calculates expression size and
expression stack height for these side-effect-free expressions.

[Lasse Reichstein, 2010-03-02 12:43:57]

LGTM if we only hit the correct type of slots.

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);
Why update the field twice, instead of just
 bitfields_ = (bitfields_ & mask) | bits
(ditto for the other two setter functions)

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) {
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.

http://codereview.chromium.org/660372/diff/1/3#newcode233
src/rewriter.cc:233: node->type() == Slot::PARAMETER) {
Could we do contexts as well? I.e., is reading a context guaranteed to be
side-effect free?

http://codereview.chromium.org/660372/diff/1/3#newcode438
src/rewriter.cc:438: node->set_stack_height(node->expression()->stack_height());
How about a combined setter that sets all three values in one go? It would also
only need to update bitflags_ once.

http://codereview.chromium.org/660372/diff/1/3#newcode538
src/rewriter.cc:538: node->left()->expression_size() + 1);
Second "left()" -> "right()".

[William Hesse, 2010-03-02 14:02:45]

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.

[Kevin Millikin (Chromium), 2010-03-02 14:33:06]

http://codereview.chromium.org/660372/diff/1004/1005
File src/ast.h (right):

http://codereview.chromium.org/660372/diff/1004/1005#newcode238
src/ast.h:238: bitfields_ = (bitfields_ & ~SideEffectFreeField::mask()) |
It's a little weird that there are three different ways of writing this used in
this same snippet (two have different indenation, one has broken it into two
statements).

http://codereview.chromium.org/660372/diff/1004/1005#newcode268
src/ast.h:268: static const int SideEffectFreeFieldStart = 0;
These constants don't help anything.  They're used in one place in a context
that should be obvious.  Write:

// Bitfields are <type, start, size>.
class SideEffectFreeField: public BitField<bool, 0, 1>;
class ExpressionSizeField: public BitField<int, 1, 5>;
class StackHeightField: public BitField<int, 32 - 6>;

If you want the constants, please use the coding style
"kSideEffectFreeeFieldStart".