From 6453056bb6407ee3ca2b31e77f9679a2d5c70055 Mon Sep 17 00:00:00 2001 From: "svenpanne@chromium.org" Date: Tue, 31 May 2011 14:37:34 +0000 Subject: [PATCH] Reduced the code ping-pong between the full code generator and contexts a bit. * Centralized AND/OR handling, keeping related code together. * Removed HandleExpression/HandleInNonTestContext and introduced VisitInSameContext instead, making it more obvious what's actually going on. * Consistently use a new context when visiting the left sub-expression of an AND/OR. Note that the context stacks in the full code generator and crankshaft are still a bit out of sync for the right sub-expression. Review URL: http://codereview.chromium.org/6976028 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@8124 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/full-codegen-arm.cc | 2 +- src/full-codegen.cc | 264 +++++++++++++++++------------------------- src/full-codegen.h | 49 +++----- src/ia32/full-codegen-ia32.cc | 2 +- src/mips/full-codegen-mips.cc | 2 +- src/x64/full-codegen-x64.cc | 2 +- 6 files changed, 125 insertions(+), 196 deletions(-) diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index 4cf650f..e39b782 100644 --- a/src/arm/full-codegen-arm.cc +++ b/src/arm/full-codegen-arm.cc @@ -4029,7 +4029,7 @@ void FullCodeGenerator::VisitForTypeofValue(Expression* expr) { context()->Plug(r0); } else { // This expression cannot throw a reference error at the top level. - context()->HandleExpression(expr); + VisitInCurrentContext(expr); } } diff --git a/src/full-codegen.cc b/src/full-codegen.cc index 67d100c..6604e41 100644 --- a/src/full-codegen.cc +++ b/src/full-codegen.cc @@ -701,143 +701,116 @@ void FullCodeGenerator::EmitInlineRuntimeCall(CallRuntime* node) { void FullCodeGenerator::VisitBinaryOperation(BinaryOperation* expr) { - Comment cmnt(masm_, "[ BinaryOperation"); - Token::Value op = expr->op(); - Expression* left = expr->left(); - Expression* right = expr->right(); - - OverwriteMode mode = NO_OVERWRITE; - if (left->ResultOverwriteAllowed()) { - mode = OVERWRITE_LEFT; - } else if (right->ResultOverwriteAllowed()) { - mode = OVERWRITE_RIGHT; - } - - switch (op) { + switch (expr->op()) { case Token::COMMA: - VisitForEffect(left); - if (context()->IsTest()) ForwardBailoutToChild(expr); - context()->HandleExpression(right); - break; - + return VisitComma(expr); case Token::OR: case Token::AND: - EmitLogicalOperation(expr); - break; - - case Token::ADD: - case Token::SUB: - case Token::DIV: - case Token::MOD: - case Token::MUL: - case Token::BIT_OR: - case Token::BIT_AND: - case Token::BIT_XOR: - case Token::SHL: - case Token::SHR: - case Token::SAR: { - // Load both operands. - VisitForStackValue(left); - VisitForAccumulatorValue(right); - - SetSourcePosition(expr->position()); - if (ShouldInlineSmiCase(op)) { - EmitInlineSmiBinaryOp(expr, op, mode, left, right); - } else { - EmitBinaryOp(expr, op, mode); - } - break; - } - + return VisitLogicalExpression(expr); default: - UNREACHABLE(); + return VisitArithmeticExpression(expr); } } -void FullCodeGenerator::EmitLogicalOperation(BinaryOperation* expr) { - Label eval_right, done; - - context()->EmitLogicalLeft(expr, &eval_right, &done); - - PrepareForBailoutForId(expr->RightId(), NO_REGISTERS); - __ bind(&eval_right); +void FullCodeGenerator::VisitComma(BinaryOperation* expr) { + Comment cmnt(masm_, "[ Comma"); + VisitForEffect(expr->left()); if (context()->IsTest()) ForwardBailoutToChild(expr); - context()->HandleExpression(expr->right()); - - __ bind(&done); + VisitInCurrentContext(expr->right()); } -void FullCodeGenerator::EffectContext::EmitLogicalLeft(BinaryOperation* expr, - Label* eval_right, - Label* done) const { - if (expr->op() == Token::OR) { - codegen()->VisitForControl(expr->left(), done, eval_right, eval_right); - } else { - ASSERT(expr->op() == Token::AND); - codegen()->VisitForControl(expr->left(), eval_right, done, eval_right); - } -} +void FullCodeGenerator::VisitLogicalExpression(BinaryOperation* expr) { + bool is_logical_and = expr->op() == Token::AND; + Comment cmnt(masm_, is_logical_and ? "[ Logical AND" : "[ Logical OR"); + Expression* left = expr->left(); + Expression* right = expr->right(); + int right_id = expr->RightId(); + Label done; + if (context()->IsTest()) { + Label eval_right; + const TestContext* test = TestContext::cast(context()); + if (is_logical_and) { + VisitForControl(left, &eval_right, test->false_label(), &eval_right); + } else { + VisitForControl(left, test->true_label(), &eval_right, &eval_right); + } + PrepareForBailoutForId(right_id, NO_REGISTERS); + __ bind(&eval_right); + ForwardBailoutToChild(expr); + + } else if (context()->IsAccumulatorValue()) { + VisitForAccumulatorValue(left); + // We want the value in the accumulator for the test, and on the stack in + // case we need it. + __ push(result_register()); + Label discard, restore; + PrepareForBailoutBeforeSplit(TOS_REG, false, NULL, NULL); + if (is_logical_and) { + DoTest(&discard, &restore, &restore); + } else { + DoTest(&restore, &discard, &restore); + } + __ bind(&restore); + __ pop(result_register()); + __ jmp(&done); + __ bind(&discard); + __ Drop(1); + PrepareForBailoutForId(right_id, NO_REGISTERS); + + } else if (context()->IsStackValue()) { + VisitForAccumulatorValue(left); + // We want the value in the accumulator for the test, and on the stack in + // case we need it. + __ push(result_register()); + Label discard; + PrepareForBailoutBeforeSplit(TOS_REG, false, NULL, NULL); + if (is_logical_and) { + DoTest(&discard, &done, &discard); + } else { + DoTest(&done, &discard, &discard); + } + __ bind(&discard); + __ Drop(1); + PrepareForBailoutForId(right_id, NO_REGISTERS); -void FullCodeGenerator::AccumulatorValueContext::EmitLogicalLeft( - BinaryOperation* expr, - Label* eval_right, - Label* done) const { - HandleExpression(expr->left()); - // We want the value in the accumulator for the test, and on the stack in case - // we need it. - __ push(result_register()); - Label discard, restore; - if (expr->op() == Token::OR) { - codegen()->PrepareForBailoutBeforeSplit(TOS_REG, false, NULL, NULL); - codegen()->DoTest(&restore, &discard, &restore); } else { - ASSERT(expr->op() == Token::AND); - codegen()->PrepareForBailoutBeforeSplit(TOS_REG, false, NULL, NULL); - codegen()->DoTest(&discard, &restore, &restore); + ASSERT(context()->IsEffect()); + Label eval_right; + if (is_logical_and) { + VisitForControl(left, &eval_right, &done, &eval_right); + } else { + VisitForControl(left, &done, &eval_right, &eval_right); + } + PrepareForBailoutForId(right_id, NO_REGISTERS); + __ bind(&eval_right); } - __ bind(&restore); - __ pop(result_register()); - __ jmp(done); - __ bind(&discard); - __ Drop(1); + + VisitInCurrentContext(right); + __ bind(&done); } -void FullCodeGenerator::StackValueContext::EmitLogicalLeft( - BinaryOperation* expr, - Label* eval_right, - Label* done) const { - codegen()->VisitForAccumulatorValue(expr->left()); - // We want the value in the accumulator for the test, and on the stack in case - // we need it. - __ push(result_register()); - Label discard; - if (expr->op() == Token::OR) { - codegen()->PrepareForBailoutBeforeSplit(TOS_REG, false, NULL, NULL); - codegen()->DoTest(done, &discard, &discard); - } else { - ASSERT(expr->op() == Token::AND); - codegen()->PrepareForBailoutBeforeSplit(TOS_REG, false, NULL, NULL); - codegen()->DoTest(&discard, done, &discard); - } - __ bind(&discard); - __ Drop(1); -} +void FullCodeGenerator::VisitArithmeticExpression(BinaryOperation* expr) { + Token::Value op = expr->op(); + Comment cmnt(masm_, "[ ArithmeticExpression"); + Expression* left = expr->left(); + Expression* right = expr->right(); + OverwriteMode mode = + left->ResultOverwriteAllowed() + ? OVERWRITE_LEFT + : (right->ResultOverwriteAllowed() ? OVERWRITE_RIGHT : NO_OVERWRITE); + VisitForStackValue(left); + VisitForAccumulatorValue(right); -void FullCodeGenerator::TestContext::EmitLogicalLeft(BinaryOperation* expr, - Label* eval_right, - Label* done) const { - if (expr->op() == Token::OR) { - codegen()->VisitForControl(expr->left(), - true_label_, eval_right, eval_right); + SetSourcePosition(expr->position()); + if (ShouldInlineSmiCase(op)) { + EmitInlineSmiBinaryOp(expr, op, mode, left, right); } else { - ASSERT(expr->op() == Token::AND); - codegen()->VisitForControl(expr->left(), - eval_right, false_label_, eval_right); + EmitBinaryOp(expr, op, mode); } } @@ -850,46 +823,23 @@ void FullCodeGenerator::ForwardBailoutToChild(Expression* expr) { } -void FullCodeGenerator::EffectContext::HandleExpression( - Expression* expr) const { - codegen()->HandleInNonTestContext(expr, NO_REGISTERS); -} - - -void FullCodeGenerator::AccumulatorValueContext::HandleExpression( - Expression* expr) const { - codegen()->HandleInNonTestContext(expr, TOS_REG); -} - - -void FullCodeGenerator::StackValueContext::HandleExpression( - Expression* expr) const { - codegen()->HandleInNonTestContext(expr, NO_REGISTERS); -} - - -void FullCodeGenerator::TestContext::HandleExpression(Expression* expr) const { - codegen()->VisitInTestContext(expr); -} - - -void FullCodeGenerator::HandleInNonTestContext(Expression* expr, State state) { - ASSERT(forward_bailout_pending_ == NULL); - AstVisitor::Visit(expr); - PrepareForBailout(expr, state); - // Forwarding bailouts to children is a one shot operation. It - // should have been processed at this point. - ASSERT(forward_bailout_pending_ == NULL); -} - - -void FullCodeGenerator::VisitInTestContext(Expression* expr) { - ForwardBailoutStack stack(expr, forward_bailout_pending_); - ForwardBailoutStack* saved = forward_bailout_stack_; - forward_bailout_pending_ = NULL; - forward_bailout_stack_ = &stack; - AstVisitor::Visit(expr); - forward_bailout_stack_ = saved; +void FullCodeGenerator::VisitInCurrentContext(Expression* expr) { + if (context()->IsTest()) { + ForwardBailoutStack stack(expr, forward_bailout_pending_); + ForwardBailoutStack* saved = forward_bailout_stack_; + forward_bailout_pending_ = NULL; + forward_bailout_stack_ = &stack; + Visit(expr); + forward_bailout_stack_ = saved; + } else { + ASSERT(forward_bailout_pending_ == NULL); + Visit(expr); + State state = context()->IsAccumulatorValue() ? TOS_REG : NO_REGISTERS; + PrepareForBailout(expr, state); + // Forwarding bailouts to children is a one shot operation. It should have + // been processed at this point. + ASSERT(forward_bailout_pending_ == NULL); + } } @@ -1287,7 +1237,7 @@ void FullCodeGenerator::VisitConditional(Conditional* expr) { for_test->false_label(), NULL); } else { - context()->HandleExpression(expr->then_expression()); + VisitInCurrentContext(expr->then_expression()); __ jmp(&done); } @@ -1296,7 +1246,7 @@ void FullCodeGenerator::VisitConditional(Conditional* expr) { if (context()->IsTest()) ForwardBailoutToChild(expr); SetExpressionPosition(expr->else_expression(), expr->else_expression_position()); - context()->HandleExpression(expr->else_expression()); + VisitInCurrentContext(expr->else_expression()); // If control flow falls through Visit, merge it with true case here. if (!context()->IsTest()) { __ bind(&done); diff --git a/src/full-codegen.h b/src/full-codegen.h index 6870c81..728a1a3 100644 --- a/src/full-codegen.h +++ b/src/full-codegen.h @@ -328,17 +328,17 @@ class FullCodeGenerator: public AstVisitor { void VisitForEffect(Expression* expr) { EffectContext context(this); - HandleInNonTestContext(expr, NO_REGISTERS); + VisitInCurrentContext(expr); } void VisitForAccumulatorValue(Expression* expr) { AccumulatorValueContext context(this); - HandleInNonTestContext(expr, TOS_REG); + VisitInCurrentContext(expr); } void VisitForStackValue(Expression* expr) { StackValueContext context(this); - HandleInNonTestContext(expr, NO_REGISTERS); + VisitInCurrentContext(expr); } void VisitForControl(Expression* expr, @@ -346,15 +346,9 @@ class FullCodeGenerator: public AstVisitor { Label* if_false, Label* fall_through) { TestContext context(this, if_true, if_false, fall_through); - VisitInTestContext(expr); - // Forwarding bailouts to children is a one shot operation. It - // should have been processed at this point. - ASSERT(forward_bailout_pending_ == NULL); + VisitInCurrentContext(expr); } - void HandleInNonTestContext(Expression* expr, State state); - void VisitInTestContext(Expression* expr); - void VisitDeclarations(ZoneList* declarations); void DeclareGlobals(Handle pairs); @@ -549,8 +543,10 @@ class FullCodeGenerator: public AstVisitor { void EmitUnaryOperation(UnaryOperation* expr, const char* comment); - // Handles the shortcutted logical binary operations in VisitBinaryOperation. - void EmitLogicalOperation(BinaryOperation* expr); + void VisitComma(BinaryOperation* expr); + void VisitLogicalExpression(BinaryOperation* expr); + void VisitArithmeticExpression(BinaryOperation* expr); + void VisitInCurrentContext(Expression* expr); void VisitForTypeofValue(Expression* expr); @@ -598,11 +594,6 @@ class FullCodeGenerator: public AstVisitor { // context. virtual void DropAndPlug(int count, Register reg) const = 0; - // For shortcutting operations || and &&. - virtual void EmitLogicalLeft(BinaryOperation* expr, - Label* eval_right, - Label* done) const = 0; - // Set up branch labels for a test expression. The three Label** parameters // are output parameters. virtual void PrepareTest(Label* materialize_true, @@ -611,12 +602,14 @@ class FullCodeGenerator: public AstVisitor { Label** if_false, Label** fall_through) const = 0; - virtual void HandleExpression(Expression* expr) const = 0; - // Returns true if we are evaluating only for side effects (ie if the result // will be discarded). virtual bool IsEffect() const { return false; } + // Returns true if we are evaluating for the value (in accu/on stack). + virtual bool IsAccumulatorValue() const { return false; } + virtual bool IsStackValue() const { return false; } + // Returns true if we are branching on the value rather than materializing // it. Only used for asserts. virtual bool IsTest() const { return false; } @@ -644,15 +637,12 @@ class FullCodeGenerator: public AstVisitor { virtual void Plug(Heap::RootListIndex) const; virtual void PlugTOS() const; virtual void DropAndPlug(int count, Register reg) const; - virtual void EmitLogicalLeft(BinaryOperation* expr, - Label* eval_right, - Label* done) const; virtual void PrepareTest(Label* materialize_true, Label* materialize_false, Label** if_true, Label** if_false, Label** fall_through) const; - virtual void HandleExpression(Expression* expr) const; + virtual bool IsAccumulatorValue() const { return true; } }; class StackValueContext : public ExpressionContext { @@ -668,15 +658,12 @@ class FullCodeGenerator: public AstVisitor { virtual void Plug(Heap::RootListIndex) const; virtual void PlugTOS() const; virtual void DropAndPlug(int count, Register reg) const; - virtual void EmitLogicalLeft(BinaryOperation* expr, - Label* eval_right, - Label* done) const; virtual void PrepareTest(Label* materialize_true, Label* materialize_false, Label** if_true, Label** if_false, Label** fall_through) const; - virtual void HandleExpression(Expression* expr) const; + virtual bool IsStackValue() const { return true; } }; class TestContext : public ExpressionContext { @@ -707,15 +694,11 @@ class FullCodeGenerator: public AstVisitor { virtual void Plug(Heap::RootListIndex) const; virtual void PlugTOS() const; virtual void DropAndPlug(int count, Register reg) const; - virtual void EmitLogicalLeft(BinaryOperation* expr, - Label* eval_right, - Label* done) const; virtual void PrepareTest(Label* materialize_true, Label* materialize_false, Label** if_true, Label** if_false, Label** fall_through) const; - virtual void HandleExpression(Expression* expr) const; virtual bool IsTest() const { return true; } private: @@ -737,15 +720,11 @@ class FullCodeGenerator: public AstVisitor { virtual void Plug(Heap::RootListIndex) const; virtual void PlugTOS() const; virtual void DropAndPlug(int count, Register reg) const; - virtual void EmitLogicalLeft(BinaryOperation* expr, - Label* eval_right, - Label* done) const; virtual void PrepareTest(Label* materialize_true, Label* materialize_false, Label** if_true, Label** if_false, Label** fall_through) const; - virtual void HandleExpression(Expression* expr) const; virtual bool IsEffect() const { return true; } }; diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index 2ab4650..03cfef4 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/src/ia32/full-codegen-ia32.cc @@ -4007,7 +4007,7 @@ void FullCodeGenerator::VisitForTypeofValue(Expression* expr) { context()->Plug(eax); } else { // This expression cannot throw a reference error at the top level. - context()->HandleExpression(expr); + VisitInCurrentContext(expr); } } diff --git a/src/mips/full-codegen-mips.cc b/src/mips/full-codegen-mips.cc index 9c93c63..4439ab4 100644 --- a/src/mips/full-codegen-mips.cc +++ b/src/mips/full-codegen-mips.cc @@ -4039,7 +4039,7 @@ void FullCodeGenerator::VisitForTypeofValue(Expression* expr) { context()->Plug(v0); } else { // This expression cannot throw a reference error at the top level. - context()->HandleExpression(expr); + VisitInCurrentContext(expr); } } diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index 686d6ae..49eed68 100644 --- a/src/x64/full-codegen-x64.cc +++ b/src/x64/full-codegen-x64.cc @@ -3983,7 +3983,7 @@ void FullCodeGenerator::VisitForTypeofValue(Expression* expr) { context()->Plug(rax); } else { // This expression cannot throw a reference error at the top level. - context()->HandleExpression(expr); + VisitInCurrentContext(expr); } } -- 2.7.4