Reduced the code ping-pong between the full code generator and contexts a bit.

src/full-codegen.cc

Index: src/full-codegen.cc
===================================================================
--- src/full-codegen.cc	(revision 8079)
+++ src/full-codegen.cc	(working copy)
@@ -702,143 +702,110 @@
 
 void FullCodeGenerator::VisitBinaryOperation(BinaryOperation* expr) {
   Comment cmnt(masm_, "[ BinaryOperation");

[Kevin Millikin (Chromium), 2011/05/31 11:46:28]

By moving this comment into VisitComma, VisitLogical..., VisitArithmetic..., you
can emit a more descriptive comment.

[Sven Panne, 2011/05/31 14:30:33]

Done.

+  switch (expr->op()) {
+    case Token::COMMA: return VisitComma(expr);
+    case Token::OR: return VisitAndOr(expr, false);

[Kevin Millikin (Chromium), 2011/05/31 11:46:28]

Let's not pass the flag here, since we can just use the expression:

case Token::OR:
case Token::AND:
  return VisitLogicalOperation(expr);
...

[Sven Panne, 2011/05/31 14:30:33]

Done, same for HGraphBuilder.

+    case Token::AND: return VisitAndOr(expr, true);
+    default: return VisitCommon(expr);
+  }
+}
+
+
+void FullCodeGenerator::VisitCommon(BinaryOperation* expr) {
   Token::Value op = expr->op();
   Expression* left = expr->left();
   Expression* right = expr->right();
+  OverwriteMode mode =
+      left->ResultOverwriteAllowed()
+      ? OVERWRITE_LEFT
+      : (right->ResultOverwriteAllowed() ? OVERWRITE_RIGHT : NO_OVERWRITE);
 
-  OverwriteMode mode = NO_OVERWRITE;
-  if (left->ResultOverwriteAllowed()) {
-    mode = OVERWRITE_LEFT;
-  } else if (right->ResultOverwriteAllowed()) {
-    mode = OVERWRITE_RIGHT;
-  }
+  VisitForStackValue(left);
+  VisitForAccumulatorValue(right);
 
-  switch (op) {
-    case Token::COMMA:
-      VisitForEffect(left);
-      if (context()->IsTest()) ForwardBailoutToChild(expr);
-      context()->HandleExpression(right);
-      break;
-
-    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;
-    }
-
-    default:
-      UNREACHABLE();
+  SetSourcePosition(expr->position());
+  if (ShouldInlineSmiCase(op)) {
+    EmitInlineSmiBinaryOp(expr, op, mode, left, right);
+  } else {
+    EmitBinaryOp(expr, op, mode);
   }
 }
 
 
-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) {
+  VisitForEffect(expr->left());
   if (context()->IsTest()) ForwardBailoutToChild(expr);
-  context()->HandleExpression(expr->right());
-
-  __ bind(&done);
+  VisitInSameContext(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::VisitAndOr(BinaryOperation* expr, bool is_logical_and) {
+  Expression* left = expr->left();
+  Expression* right = expr->right();
+  int rightId = expr->RightId();

[Kevin Millikin (Chromium), 2011/05/31 11:46:28]

right_id

[Sven Panne, 2011/05/31 14:30:33]

Done.

+  Label done;
 
+  if (context()->IsTest()) {

[Kevin Millikin (Chromium), 2011/05/31 11:46:28]

I did like having this code as virtual functions on the context classes.

[Sven Panne, 2011/05/31 14:30:33]

In principle I agree that the explicit IfFooContext() calls are not nice, but
moving the if/then/else branches to the contexts would result in a heavy feature
envy regarding the full code generator. Removing this feature envy was basically
the main motivation for this change. Furthermore, similar ifs are elsewhere, so
this is more consistent.

In a nutshell: We should improve how we handle the triple dispatch, but not in
this change.

+    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(rightId, NO_REGISTERS);
+    __ bind(&eval_right);
+    ForwardBailoutToChild(expr);
 
-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);
-  }
-  __ bind(&restore);
-  __ pop(result_register());
-  __ jmp(done);
-  __ bind(&discard);
-  __ Drop(1);
-}
+  } 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(rightId, 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(rightId, NO_REGISTERS);
 
-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);
+    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(rightId, NO_REGISTERS);
+    __ bind(&eval_right);
   }
-  __ bind(&discard);
-  __ Drop(1);
-}
 
-
-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);
-  } else {
-    ASSERT(expr->op() == Token::AND);
-    codegen()->VisitForControl(expr->left(),
-                               eval_right, false_label_, eval_right);
-  }
+  VisitInSameContext(right);
+  __ bind(&done);
 }
 
 
@@ -850,49 +817,26 @@
 }
 
 
-void FullCodeGenerator::EffectContext::HandleExpression(
-    Expression* expr) const {
-  codegen()->HandleInNonTestContext(expr, NO_REGISTERS);
+void FullCodeGenerator::VisitInSameContext(Expression* expr) {
+  if (context()->IsTest()) {
+    ForwardBailoutStack stack(expr, forward_bailout_pending_);
+    ForwardBailoutStack* saved = forward_bailout_stack_;
+    forward_bailout_pending_ = NULL;
+    forward_bailout_stack_ = &stack;
+    AstVisitor::Visit(expr);

[Kevin Millikin (Chromium), 2011/05/31 11:46:28]

This can just be Visit(expr), can't it?

[Sven Panne, 2011/05/31 14:30:33]

Done.

+    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);
+  }
 }
 
 
-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::VisitBlock(Block* stmt) {
   Comment cmnt(masm_, "[ Block");
   Breakable nested_statement(this, stmt);
@@ -1287,7 +1231,7 @@
                     for_test->false_label(),
                     NULL);
   } else {
-    context()->HandleExpression(expr->then_expression());
+    VisitInSameContext(expr->then_expression());
     __ jmp(&done);
   }
 
@@ -1296,7 +1240,7 @@
   if (context()->IsTest()) ForwardBailoutToChild(expr);
   SetExpressionPosition(expr->else_expression(),
                         expr->else_expression_position());
-  context()->HandleExpression(expr->else_expression());
+  VisitInSameContext(expr->else_expression());
   // If control flow falls through Visit, merge it with true case here.
   if (!context()->IsTest()) {
     __ bind(&done);