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

src/full-codegen.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 683 matching lines @@
   const Runtime::Function* function = node->function();
   ASSERT(function != NULL);
   ASSERT(function->intrinsic_type == Runtime::INLINE);
   InlineFunctionGenerator generator =
       FindInlineFunctionGenerator(function->function_id);
   ((*this).*(generator))(args);
 }
 
 
 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.

-  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) {
-    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();
+  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::EmitLogicalOperation(BinaryOperation* expr) {
-  Label eval_right, done;
+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);
 
-  context()->EmitLogicalLeft(expr, &eval_right, &done);
+  VisitForStackValue(left);
+  VisitForAccumulatorValue(right);
 
-  PrepareForBailoutForId(expr->RightId(), NO_REGISTERS);
-  __ bind(&eval_right);
+  SetSourcePosition(expr->position());
+  if (ShouldInlineSmiCase(op)) {
+    EmitInlineSmiBinaryOp(expr, op, mode, left, right);
+  } else {
+    EmitBinaryOp(expr, op, mode);
+  }
+}
+
+
+void FullCodeGenerator::VisitComma(BinaryOperation* expr) {
+  VisitForEffect(expr->left());
   if (context()->IsTest()) ForwardBailoutToChild(expr);
-  context()->HandleExpression(expr->right());
+  VisitInSameContext(expr->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);
+
+  } 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);
+
+  } else {
+    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);
+  }
+
+  VisitInSameContext(right);
   __ bind(&done);
 }
 
 
-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::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);
-}
-
-
-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::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);
-  }
-}
-
-
 void FullCodeGenerator::ForwardBailoutToChild(Expression* expr) {
   if (!info_->HasDeoptimizationSupport()) return;
   ASSERT(context()->IsTest());
   ASSERT(expr == forward_bailout_stack_->expr());
   forward_bailout_pending_ = forward_bailout_stack_;
 }
 
 
-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::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::VisitBlock(Block* stmt) {
   Comment cmnt(masm_, "[ Block");
   Breakable nested_statement(this, stmt);
   SetStatementPosition(stmt);
 
   PrepareForBailoutForId(stmt->EntryId(), NO_REGISTERS);
   VisitStatements(stmt->statements());
@@ skipping 377 matching lines @@
   __ bind(&true_case);
   SetExpressionPosition(expr->then_expression(),
                         expr->then_expression_position());
   if (context()->IsTest()) {
     const TestContext* for_test = TestContext::cast(context());
     VisitForControl(expr->then_expression(),
                     for_test->true_label(),
                     for_test->false_label(),
                     NULL);
   } else {
-    context()->HandleExpression(expr->then_expression());
+    VisitInSameContext(expr->then_expression());
     __ jmp(&done);
   }
 
   PrepareForBailoutForId(expr->ElseId(), NO_REGISTERS);
   __ bind(&false_case);
   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);
   }
 }
 
 
 void FullCodeGenerator::VisitLiteral(Literal* expr) {
   Comment cmnt(masm_, "[ Literal");
   context()->Plug(expr->handle());
@@ skipping 55 matching lines @@
   __ Drop(stack_depth);
   __ PopTryHandler();
   return 0;
 }
 
 
 #undef __
 
 
 } }  // namespace v8::internal