Fix issue 977, occasional failure of the DeltaBlue benchmark.

Before, when we deoptimized after a branch we jumped to before the branch
was taken in the unoptimized code with a token value that indicated when
edge to take.  There was a lot of machinery to track this value through the
short-circuit logical operations and logical negation, and to handle it
properly at inline function return sites.  There was also machinery to
prevent incorrectly seeing this environment with the extra value never
actually materialized in the unoptimized code.

Instead, now we deoptimize directly to one of the targets of the branch.
Much but not yet all of the extra machinery has been removed or simplified.
The cost is that branching control structures (the looping statements, if
statements, conditional expressions, and the short-circuit binary logical
operations) need extra AST IDs to identify the branch targets.

Review URL: http://codereview.chromium.org/5908001

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@6049 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/ast-inl.h

@@ -71,14 +71,16 @@ DoWhileStatement::DoWhileStatement(ZoneStringList* labels)
     : IterationStatement(labels),
       cond_(NULL),
       condition_position_(-1),
-      next_id_(GetNextId()) {
+      continue_id_(GetNextId()),
+      back_edge_id_(GetNextId()) {
 }
 
 
 WhileStatement::WhileStatement(ZoneStringList* labels)
     : IterationStatement(labels),
       cond_(NULL),
-      may_have_function_literal_(true) {
+      may_have_function_literal_(true),
+      body_id_(GetNextId()) {
 }
 
 
@@ -89,7 +91,8 @@ ForStatement::ForStatement(ZoneStringList* labels)
       next_(NULL),
       may_have_function_literal_(true),
       loop_variable_(NULL),
-      next_id_(GetNextId()) {
+      continue_id_(GetNextId()),
+      body_id_(GetNextId()) {
 }
 
 

src/ast.h

@@ -476,12 +476,14 @@ class DoWhileStatement: public IterationStatement {
   void set_condition_position(int pos) { condition_position_ = pos; }
 
   // Bailout support.
-  virtual int ContinueId() const { return next_id_; }
+  virtual int ContinueId() const { return continue_id_; }
+  int BackEdgeId() const { return back_edge_id_; }
 
  private:
   Expression* cond_;
   int condition_position_;
-  int next_id_;
+  int continue_id_;
+  int back_edge_id_;
 };
 
 
@@ -506,11 +508,13 @@ class WhileStatement: public IterationStatement {
 
   // Bailout support.
   virtual int ContinueId() const { return EntryId(); }
+  int BodyId() const { return body_id_; }
 
  private:
   Expression* cond_;
   // True if there is a function literal subexpression in the condition.
   bool may_have_function_literal_;
+  int body_id_;
 };
 
 
@@ -542,7 +546,8 @@ class ForStatement: public IterationStatement {
   }
 
   // Bailout support.
-  virtual int ContinueId() const { return next_id_; }
+  virtual int ContinueId() const { return continue_id_; }
+  int BodyId() const { return body_id_; }
 
   bool is_fast_smi_loop() { return loop_variable_ != NULL; }
   Variable* loop_variable() { return loop_variable_; }
@@ -555,7 +560,8 @@ class ForStatement: public IterationStatement {
   // True if there is a function literal subexpression in the condition.
   bool may_have_function_literal_;
   Variable* loop_variable_;
-  int next_id_;
+  int continue_id_;
+  int body_id_;
 };
 
 
@@ -735,7 +741,10 @@ class IfStatement: public Statement {
               Statement* else_statement)
       : condition_(condition),
         then_statement_(then_statement),
-        else_statement_(else_statement) { }
+        else_statement_(else_statement),
+        then_id_(GetNextId()),
+        else_id_(GetNextId()) {
+  }
 
   DECLARE_NODE_TYPE(IfStatement)
 
@@ -748,10 +757,15 @@ class IfStatement: public Statement {
   Statement* then_statement() const { return then_statement_; }
   Statement* else_statement() const { return else_statement_; }
 
+  int ThenId() const { return then_id_; }
+  int ElseId() const { return else_id_; }
+
  private:
   Expression* condition_;
   Statement* then_statement_;
   Statement* else_statement_;
+  int then_id_;
+  int else_id_;
 };
 
 
@@ -1376,6 +1390,9 @@ class BinaryOperation: public Expression {
                   int pos)
       : op_(op), left_(left), right_(right), pos_(pos), is_smi_only_(false) {
     ASSERT(Token::IsBinaryOp(op));
+    right_id_ = (op == Token::AND || op == Token::OR)
+        ? GetNextId()
+        : AstNode::kNoNumber;
   }
 
   // Create the binary operation corresponding to a compound assignment.
@@ -1396,12 +1413,18 @@ class BinaryOperation: public Expression {
   void RecordTypeFeedback(TypeFeedbackOracle* oracle);
   bool IsSmiOnly() const { return is_smi_only_; }
 
+  // Bailout support.
+  int RightId() const { return right_id_; }
+
  private:
   Token::Value op_;
   Expression* left_;
   Expression* right_;
   int pos_;
   bool is_smi_only_;
+  // The short-circuit logical operations have an AST ID for their
+  // right-hand subexpression.
+  int right_id_;
 };
 
 
@@ -1526,7 +1549,10 @@ class Conditional: public Expression {
         then_expression_(then_expression),
         else_expression_(else_expression),
         then_expression_position_(then_expression_position),
-        else_expression_position_(else_expression_position) { }
+        else_expression_position_(else_expression_position),
+        then_id_(GetNextId()),
+        else_id_(GetNextId()) {
+  }
 
   DECLARE_NODE_TYPE(Conditional)
 
@@ -1536,8 +1562,11 @@ class Conditional: public Expression {
   Expression* then_expression() const { return then_expression_; }
   Expression* else_expression() const { return else_expression_; }
 
-  int then_expression_position() { return then_expression_position_; }
-  int else_expression_position() { return else_expression_position_; }
+  int then_expression_position() const { return then_expression_position_; }
+  int else_expression_position() const { return else_expression_position_; }
+
+  int ThenId() const { return then_id_; }
+  int ElseId() const { return else_id_; }
 
  private:
   Expression* condition_;
@@ -1545,6 +1574,8 @@ class Conditional: public Expression {
   Expression* else_expression_;
   int then_expression_position_;
   int else_expression_position_;
+  int then_id_;
+  int else_id_;
 };
 
 

src/full-codegen.cc

@@ -761,6 +761,7 @@ void FullCodeGenerator::EmitLogicalOperation(BinaryOperation* expr) {
 
   context()->EmitLogicalLeft(expr, &eval_right, &done);
 
+  PrepareForBailoutForId(expr->RightId(), NO_REGISTERS);
   __ bind(&eval_right);
   if (context()->IsTest()) ForwardBailoutToChild(expr);
   context()->HandleExpression(expr->right());
@@ -925,16 +926,21 @@ void FullCodeGenerator::VisitIfStatement(IfStatement* stmt) {
 
   if (stmt->HasElseStatement()) {
     VisitForControl(stmt->condition(), &then_part, &else_part, &then_part);
+    PrepareForBailoutForId(stmt->ThenId(), NO_REGISTERS);
     __ bind(&then_part);
     Visit(stmt->then_statement());
     __ jmp(&done);
 
+    PrepareForBailoutForId(stmt->ElseId(), NO_REGISTERS);
     __ bind(&else_part);
     Visit(stmt->else_statement());
   } else {
     VisitForControl(stmt->condition(), &then_part, &done, &then_part);
+    PrepareForBailoutForId(stmt->ThenId(), NO_REGISTERS);
     __ bind(&then_part);
     Visit(stmt->then_statement());
+
+    PrepareForBailoutForId(stmt->ElseId(), NO_REGISTERS);
   }
   __ bind(&done);
   PrepareForBailoutForId(stmt->id(), NO_REGISTERS);
@@ -1053,12 +1059,13 @@ void FullCodeGenerator::VisitDoWhileStatement(DoWhileStatement* stmt) {
                   &stack_check);
 
   // Check stack before looping.
+  PrepareForBailoutForId(stmt->BackEdgeId(), NO_REGISTERS);
   __ bind(&stack_check);
   EmitStackCheck(stmt);
   __ jmp(&body);
 
-  __ bind(loop_statement.break_target());
   PrepareForBailoutForId(stmt->ExitId(), NO_REGISTERS);
+  __ bind(loop_statement.break_target());
   decrement_loop_depth();
 }
 
@@ -1073,6 +1080,7 @@ void FullCodeGenerator::VisitWhileStatement(WhileStatement* stmt) {
   // Emit the test at the bottom of the loop.
   __ jmp(&test);
 
+  PrepareForBailoutForId(stmt->BodyId(), NO_REGISTERS);
   __ bind(&body);
   Visit(stmt->body());
 
@@ -1090,8 +1098,8 @@ void FullCodeGenerator::VisitWhileStatement(WhileStatement* stmt) {
                   loop_statement.break_target(),
                   loop_statement.break_target());
 
-  __ bind(loop_statement.break_target());
   PrepareForBailoutForId(stmt->ExitId(), NO_REGISTERS);
+  __ bind(loop_statement.break_target());
   decrement_loop_depth();
 }
 
@@ -1109,12 +1117,12 @@ void FullCodeGenerator::VisitForStatement(ForStatement* stmt) {
   // Emit the test at the bottom of the loop (even if empty).
   __ jmp(&test);
 
+  PrepareForBailoutForId(stmt->BodyId(), NO_REGISTERS);
   __ bind(&body);
   Visit(stmt->body());
 
-  __ bind(loop_statement.continue_target());
   PrepareForBailoutForId(stmt->ContinueId(), NO_REGISTERS);
-
+  __ bind(loop_statement.continue_target());
   SetStatementPosition(stmt);
   if (stmt->next() != NULL) {
     Visit(stmt->next());
@@ -1137,8 +1145,8 @@ void FullCodeGenerator::VisitForStatement(ForStatement* stmt) {
     __ jmp(&body);
   }
 
-  __ bind(loop_statement.break_target());
   PrepareForBailoutForId(stmt->ExitId(), NO_REGISTERS);
+  __ bind(loop_statement.break_target());
   decrement_loop_depth();
 }
 
@@ -1269,6 +1277,7 @@ void FullCodeGenerator::VisitConditional(Conditional* expr) {
   Label true_case, false_case, done;
   VisitForControl(expr->condition(), &true_case, &false_case, &true_case);
 
+  PrepareForBailoutForId(expr->ThenId(), NO_REGISTERS);
   __ bind(&true_case);
   SetExpressionPosition(expr->then_expression(),
                         expr->then_expression_position());
@@ -1283,6 +1292,7 @@ void FullCodeGenerator::VisitConditional(Conditional* expr) {
     __ jmp(&done);
   }
 
+  PrepareForBailoutForId(expr->ElseId(), NO_REGISTERS);
   __ bind(&false_case);
   if (context()->IsTest()) ForwardBailoutToChild(expr);
   SetExpressionPosition(expr->else_expression(),

src/hydrogen-instructions.h

@@ -94,13 +94,13 @@ class LChunkBuilder;
 //     HCallStub
 //     HConstant
 //     HControlInstruction
+//       HDeoptimize
 //       HGoto
 //       HUnaryControlInstruction
 //         HBranch
 //         HCompareMapAndBranch
 //         HReturn
 //         HThrow
-//     HDeoptimize
 //     HEnterInlined
 //     HFunctionLiteral
 //     HGlobalObject

src/hydrogen.h

@@ -136,14 +136,6 @@ class HBasicBlock: public ZoneObject {
   bool IsInlineReturnTarget() const { return is_inline_return_target_; }
   void MarkAsInlineReturnTarget() { is_inline_return_target_ = true; }
 
-  // If this block is a successor of a branch, his flags tells whether the
-  // preceding branch was inverted or not.
-  bool inverted() { return inverted_; }
-  void set_inverted(bool b) { inverted_ = b; }
-
-  HBasicBlock* deopt_predecessor() { return deopt_predecessor_; }
-  void set_deopt_predecessor(HBasicBlock* block) { deopt_predecessor_ = block; }
-
   Handle<Object> cond() { return cond_; }
   void set_cond(Handle<Object> value) { cond_ = value; }
 
@@ -176,8 +168,6 @@ class HBasicBlock: public ZoneObject {
   ZoneList<int> deleted_phis_;
   SetOncePointer<HBasicBlock> parent_loop_header_;
   bool is_inline_return_target_;
-  bool inverted_;
-  HBasicBlock* deopt_predecessor_;
   Handle<Object> cond_;
 };
 
@@ -615,14 +605,10 @@ class TestContext: public AstContext {
  public:
   TestContext(HGraphBuilder* owner,
               HBasicBlock* if_true,
-              HBasicBlock* if_false,
-              bool invert_true,
-              bool invert_false)
+              HBasicBlock* if_false)
       : AstContext(owner, Expression::kTest),
         if_true_(if_true),
-        if_false_(if_false),
-        invert_true_(invert_true),
-        invert_false_(invert_false) {
+        if_false_(if_false) {
   }
 
   virtual void ReturnValue(HValue* value);
@@ -636,9 +622,6 @@ class TestContext: public AstContext {
   HBasicBlock* if_true() const { return if_true_; }
   HBasicBlock* if_false() const { return if_false_; }
 
-  bool invert_true() { return invert_true_; }
-  bool invert_false() { return invert_false_; }
-
  private:
   // Build the shared core part of the translation unpacking a value into
   // control flow.
@@ -646,8 +629,6 @@ class TestContext: public AstContext {
 
   HBasicBlock* if_true_;
   HBasicBlock* if_false_;
-  bool invert_true_;
-  bool invert_false_;
 };
 
 
@@ -723,10 +704,6 @@ class HGraphBuilder: public AstVisitor {
   void AddToSubgraph(HSubgraph* graph, ZoneList<Statement*>* stmts);
   void AddToSubgraph(HSubgraph* graph, Statement* stmt);
   void AddToSubgraph(HSubgraph* graph, Expression* expr);
-  void AddConditionToSubgraph(HSubgraph* subgraph,
-                              Expression* expr,
-                              HSubgraph* true_graph,
-                              HSubgraph* false_graph);
 
   HValue* Top() const { return environment()->Top(); }
   void Drop(int n) { environment()->Drop(n); }
@@ -736,17 +713,8 @@ class HGraphBuilder: public AstVisitor {
   void VisitForEffect(Expression* expr);
   void VisitForControl(Expression* expr,
                        HBasicBlock* true_block,
-                       HBasicBlock* false_block,
-                       bool invert_true,
-                       bool invert_false);
-
-  // Visit an expression in a 'condition' context, i.e., in a control
-  // context but not a subexpression of logical and, or, or not.
-  void VisitCondition(Expression* expr,
-                      HBasicBlock* true_graph,
-                      HBasicBlock* false_graph,
-                      bool invert_true,
-                      bool invert_false);
+                       HBasicBlock* false_block);
+
   // Visit an argument and wrap it in a PushArgument instruction.
   HValue* VisitArgument(Expression* expr);
   void VisitArgumentList(ZoneList<Expression*>* arguments);

src/ia32/lithium-ia32.cc

@@ -881,19 +881,6 @@ LOperand* LChunkBuilder::FixedTemp(XMMRegister reg) {
 
 
 LInstruction* LChunkBuilder::DoBlockEntry(HBlockEntry* instr) {
-  HBasicBlock* deopt_predecessor = instr->block()->deopt_predecessor();
-  if (deopt_predecessor != NULL &&
-      deopt_predecessor->inverted()) {
-    HEnvironment* env = current_block_->last_environment();
-    HValue* value = env->Pop();
-    ASSERT(value->IsConstant());
-    Handle<Object> obj = HConstant::cast(value)->handle();
-    ASSERT(*obj == *Factory::true_value() || *obj == *Factory::false_value());
-    env->Push(*obj == *Factory::true_value()
-              ? current_block_->graph()->GetConstantFalse()
-              : current_block_->graph()->GetConstantTrue());
-  }
-
   return new LLabel(instr->block());
 }