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

diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc
index 4cf650f..e39b782 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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<Declaration*>* declarations);
   void DeclareGlobals(Handle<FixedArray> 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);
   }
 }