Refactor handling of test expressions in the graph builder.

src/hydrogen.cc

Index: src/hydrogen.cc
===================================================================
--- src/hydrogen.cc	(revision 8460)
+++ src/hydrogen.cc	(working copy)
@@ -886,9 +886,8 @@
  private:
   void TraceRange(const char* msg, ...);
   void Analyze(HBasicBlock* block);
-  void InferControlFlowRange(HTest* test, HBasicBlock* dest);
-  void InferControlFlowRange(Token::Value op, HValue* value, HValue* other);
-  void InferPhiRange(HPhi* phi);
+  void InferControlFlowRange(HCompareIDAndBranch* test, HBasicBlock* dest);
+  void UpdateControlFlowRange(Token::Value op, HValue* value, HValue* other);
   void InferRange(HValue* value);
   void RollBackTo(int index);
   void AddRange(HValue* value, Range* range);
@@ -922,15 +921,15 @@
   // Infer range based on control flow.
   if (block->predecessors()->length() == 1) {
     HBasicBlock* pred = block->predecessors()->first();
-    if (pred->end()->IsTest()) {
-      InferControlFlowRange(HTest::cast(pred->end()), block);
+    if (pred->end()->IsCompareIDAndBranch()) {
+      InferControlFlowRange(HCompareIDAndBranch::cast(pred->end()), block);
     }
   }
 
   // Process phi instructions.
   for (int i = 0; i < block->phis()->length(); ++i) {
     HPhi* phi = block->phis()->at(i);
-    InferPhiRange(phi);
+    InferRange(phi);
   }
 
   // Go through all instructions of the current block.
@@ -949,28 +948,26 @@
 }
 
 
-void HRangeAnalysis::InferControlFlowRange(HTest* test, HBasicBlock* dest) {
+void HRangeAnalysis::InferControlFlowRange(HCompareIDAndBranch* test,
+                                           HBasicBlock* dest) {
   ASSERT((test->FirstSuccessor() == dest) == (test->SecondSuccessor() != dest));
-  if (test->value()->IsCompare()) {
-    HCompare* compare = HCompare::cast(test->value());
-    if (compare->GetInputRepresentation().IsInteger32()) {
-      Token::Value op = compare->token();
-      if (test->SecondSuccessor() == dest) {
-        op = Token::NegateCompareOp(op);
-      }
-      Token::Value inverted_op = Token::InvertCompareOp(op);
-      InferControlFlowRange(op, compare->left(), compare->right());
-      InferControlFlowRange(inverted_op, compare->right(), compare->left());
+  if (test->GetInputRepresentation().IsInteger32()) {
+    Token::Value op = test->token();
+    if (test->SecondSuccessor() == dest) {
+      op = Token::NegateCompareOp(op);
     }
+    Token::Value inverted_op = Token::InvertCompareOp(op);
+    UpdateControlFlowRange(op, test->left(), test->right());
+    UpdateControlFlowRange(inverted_op, test->right(), test->left());
   }
 }
 
 
 // We know that value [op] other. Use this information to update the range on
 // value.
-void HRangeAnalysis::InferControlFlowRange(Token::Value op,
-                                           HValue* value,
-                                           HValue* other) {
+void HRangeAnalysis::UpdateControlFlowRange(Token::Value op,
+                                            HValue* value,
+                                            HValue* other) {
   Range temp_range;
   Range* range = other->range() != NULL ? other->range() : &temp_range;
   Range* new_range = NULL;
@@ -1001,12 +998,6 @@
 }
 
 
-void HRangeAnalysis::InferPhiRange(HPhi* phi) {
-  // TODO(twuerthinger): Infer loop phi ranges.
-  InferRange(phi);
-}
-
-
 void HRangeAnalysis::InferRange(HValue* value) {
   ASSERT(!value->HasRange());
   if (!value->representation().IsNone()) {
@@ -1940,7 +1931,7 @@
   HPhase phase("MarkDeoptimizeOnUndefined", this);
   // Compute DeoptimizeOnUndefined flag for phis.
   // Any phi that can reach a use with DeoptimizeOnUndefined set must
-  // have DeoptimizeOnUndefined set.  Currently only HCompare, with
+  // have DeoptimizeOnUndefined set.  Currently only HCompareIDAndBranch, with
   // double input representation, has this flag set.
   // The flag is used by HChange tagged->double, which must deoptimize
   // if one of its uses has this flag set.
@@ -2073,52 +2064,68 @@
 
 
 void TestContext::ReturnValue(HValue* value) {
-  BuildBranch(value);
+  BuildBranch(value, new HTest(value));
 }
 
 
 void EffectContext::ReturnInstruction(HInstruction* instr, int ast_id) {

[Kevin Millikin (Chromium), 2011/06/30 10:52:03]

An alternative that I like better is to add a method void
AstContext::ReturnControl(HControlInstruction* instr, int ast_id) and avoid the
testing in these methods (even assert !instr->IsControlInstruction()).  I think
we should always know at the call site whether we have a control instruction or
not.  (Alternative names: ReturnSplit, ReturnBranch.)

[fschneider, 2011/06/30 13:13:36]

Done.

-  owner()->AddInstruction(instr);
-  if (instr->HasSideEffects()) owner()->AddSimulate(ast_id);
+  if (!instr->IsControlInstruction()) {
+    owner()->AddInstruction(instr);
+    if (instr->HasSideEffects()) owner()->AddSimulate(ast_id);
+  } else {
+    ASSERT(!instr->HasSideEffects());
+    owner()->MaterializeBoolean(HControlInstruction::cast(instr), ast_id);

[Kevin Millikin (Chromium), 2011/06/30 10:52:03]

I wonder why it is necessary to materialize anything in an effect context?  I
can see needing the branch in our implementation, but not the Phi(true, false)
at the join.

[fschneider, 2011/06/30 13:13:36]

Done. Removed MaterializeBoolean.

+  }
 }
 
 
 void ValueContext::ReturnInstruction(HInstruction* instr, int ast_id) {
   if (!arguments_allowed() && instr->CheckFlag(HValue::kIsArguments)) {
-    owner()->Bailout("bad value context for arguments object value");
+    return owner()->Bailout("bad value context for arguments object value");
   }
-  owner()->AddInstruction(instr);
-  owner()->Push(instr);
-  if (instr->HasSideEffects()) owner()->AddSimulate(ast_id);
+  if (!instr->IsControlInstruction()) {
+    owner()->AddInstruction(instr);
+    owner()->Push(instr);
+    if (instr->HasSideEffects()) owner()->AddSimulate(ast_id);
+  } else {
+    ASSERT(!instr->HasSideEffects());
+    owner()->MaterializeBoolean(HControlInstruction::cast(instr), ast_id);
+  }
 }
 
 
 void TestContext::ReturnInstruction(HInstruction* instr, int ast_id) {
-  HGraphBuilder* builder = owner();
-  builder->AddInstruction(instr);
-  // We expect a simulate after every expression with side effects, though
-  // this one isn't actually needed (and wouldn't work if it were targeted).
-  if (instr->HasSideEffects()) {
-    builder->Push(instr);
-    builder->AddSimulate(ast_id);
-    builder->Pop();
+  if (!instr->IsControlInstruction()) {
+    HGraphBuilder* builder = owner();
+    builder->AddInstruction(instr);
+    // We expect a simulate after every expression with side effects, though
+    // this one isn't actually needed (and wouldn't work if it were targeted).
+    if (instr->HasSideEffects()) {
+      builder->Push(instr);
+      builder->AddSimulate(ast_id);
+      builder->Pop();
+    }
+    BuildBranch(instr, new HTest(instr));
+  } else {
+    BuildBranch(NULL, HControlInstruction::cast(instr));
   }
-  BuildBranch(instr);
 }
 
 
-void TestContext::BuildBranch(HValue* value) {
+void TestContext::BuildBranch(HValue* value, HControlInstruction* test) {

[Kevin Millikin (Chromium), 2011/06/30 10:52:03]

This is trying to do too much.  It's strange that it takes either NULL and a
control instruction or else a non-control instruction value and an HTest of that
same value.

I suggest restoring the original BuildBranch to branch on a value, and just
inlining the case of BuildBranch(NULL, instr) at the one call site.  You're not
really "building a branch" there anyway, you've already got a branch, so you're
building a join.

[fschneider, 2011/06/30 13:13:36]

Done.

   // We expect the graph to be in edge-split form: there is no edge that
   // connects a branch node to a join node.  We conservatively ensure that
   // property by always adding an empty block on the outgoing edges of this
   // branch.
   HGraphBuilder* builder = owner();
-  if (value->CheckFlag(HValue::kIsArguments)) {
+  ASSERT(value == NULL || !value->IsControlInstruction());
+  if (value != NULL && value->CheckFlag(HValue::kIsArguments)) {
     builder->Bailout("arguments object value in a test context");
   }
   HBasicBlock* empty_true = builder->graph()->CreateBasicBlock();
   HBasicBlock* empty_false = builder->graph()->CreateBasicBlock();
-  HTest* test = new(zone()) HTest(value, empty_true, empty_false);
+  test->SetSuccessorAt(0, empty_true);
+  test->SetSuccessorAt(1, empty_false);
   builder->current_block()->Finish(test);
 
   empty_true->Goto(if_true());
@@ -2624,15 +2631,16 @@
     // Otherwise generate a compare and branch.
     CHECK_ALIVE(VisitForValue(clause->label()));
     HValue* label_value = Pop();
-    HCompare* compare =
-        new(zone()) HCompare(tag_value, label_value, Token::EQ_STRICT);
+    HCompareIDAndBranch* compare =
+        new(zone()) HCompareIDAndBranch(tag_value,
+                                        label_value,
+                                        Token::EQ_STRICT);
     compare->SetInputRepresentation(Representation::Integer32());
-    ASSERT(!compare->HasSideEffects());
-    AddInstruction(compare);
     HBasicBlock* body_block = graph()->CreateBasicBlock();
     HBasicBlock* next_test_block = graph()->CreateBasicBlock();
-    HTest* branch = new(zone()) HTest(compare, body_block, next_test_block);
-    current_block()->Finish(branch);
+    compare->SetSuccessorAt(0, body_block);
+    compare->SetSuccessorAt(1, next_test_block);
+    current_block()->Finish(compare);
     set_current_block(next_test_block);
   }
 
@@ -3955,23 +3963,24 @@
     if (type_todo[elements_kind]) {
       HBasicBlock* if_true = graph()->CreateBasicBlock();
       HBasicBlock* if_false = graph()->CreateBasicBlock();
-      HCompareConstantEq* compare = new(zone()) HCompareConstantEq(
-          elements_kind_instr,
-          elements_kind,
-          Token::EQ_STRICT);
-      AddInstruction(compare);
-      HTest* branch = new(zone()) HTest(compare, if_true, if_false);
-      current_block()->Finish(branch);
+      HCompareConstantEqAndBranch* compare =
+          new(zone()) HCompareConstantEqAndBranch(elements_kind_instr,
+                                                  elements_kind,
+                                                  Token::EQ_STRICT);
+      compare->SetSuccessorAt(0, if_true);
+      compare->SetSuccessorAt(1, if_false);
+      current_block()->Finish(compare);
 
       set_current_block(if_true);
       HInstruction* access;
       if (elements_kind == JSObject::FAST_ELEMENTS) {
         HBasicBlock* if_jsarray = graph()->CreateBasicBlock();
         HBasicBlock* if_fastobject = graph()->CreateBasicBlock();
-        HInstruction* typecheck =
-            AddInstruction(new(zone()) HHasInstanceType(object, JS_ARRAY_TYPE));
-        HTest* test = new(zone()) HTest(typecheck, if_jsarray, if_fastobject);
-        current_block()->Finish(test);
+        HHasInstanceTypeAndBranch* typecheck =
+            new(zone()) HHasInstanceTypeAndBranch(object, JS_ARRAY_TYPE);
+        typecheck->SetSuccessorAt(0, if_jsarray);
+        typecheck->SetSuccessorAt(1, if_fastobject);
+        current_block()->Finish(typecheck);
 
         set_current_block(if_jsarray);
         HInstruction* length = new(zone()) HJSArrayLength(object);
@@ -5494,7 +5503,7 @@
                                                Handle<String> check) {
   CHECK_ALIVE(VisitForTypeOf(expr));
   HValue* expr_value = Pop();
-  HInstruction* instr = new(zone()) HTypeofIs(expr_value, check);
+  HTypeofIsAndBranch* instr = new(zone()) HTypeofIsAndBranch(expr_value, check);
   instr->set_position(compare_expr->position());
   ast_context()->ReturnInstruction(instr, compare_expr->id());
 }
@@ -5505,8 +5514,8 @@
   CHECK_ALIVE(VisitForValue(expr));
   HValue* lhs = Pop();
   HValue* rhs = graph()->GetConstantUndefined();
-  HInstruction* instr =
-      new(zone()) HCompareObjectEq(lhs, rhs);
+  HCompareObjectEqAndBranch* instr =
+      new(zone()) HCompareObjectEqAndBranch(lhs, rhs);
   instr->set_position(compare_expr->position());
   ast_context()->ReturnInstruction(instr, compare_expr->id());
 }
@@ -5518,11 +5527,13 @@
   ASSERT(current_block()->HasPredecessor());
   if (IsClassOfTest(expr)) {
     CallRuntime* call = expr->left()->AsCallRuntime();
+    ASSERT(call->arguments()->length() == 1);
     CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
     HValue* value = Pop();
     Literal* literal = expr->right()->AsLiteral();
     Handle<String> rhs = Handle<String>::cast(literal->handle());
-    HInstruction* instr = new(zone()) HClassOfTest(value, rhs);
+    HClassOfTestAndBranch* instr =
+        new(zone()) HClassOfTestAndBranch(value, rhs);
     instr->set_position(expr->position());
     ast_context()->ReturnInstruction(instr, expr->id());
     return;
@@ -5556,7 +5567,6 @@
   HValue* left = Pop();
   Token::Value op = expr->op();
 
-  HInstruction* instr = NULL;
   if (op == Token::INSTANCEOF) {
     // Check to see if the rhs of the instanceof is a global function not
     // residing in new space. If it is we assume that the function will stay the
@@ -5587,13 +5597,23 @@
     // assumed to stay the same for this instanceof.
     if (target.is_null()) {
       HValue* context = environment()->LookupContext();
-      instr = new(zone()) HInstanceOf(context, left, right);
+      HInstanceOf* result = new(zone()) HInstanceOf(context, left, right);
+      result->set_position(expr->position());
+      ast_context()->ReturnInstruction(result, expr->id());

[Kevin Millikin (Chromium), 2011/06/30 10:52:03]

Maybe we should take to writing

return ast_context()->ReturnInstruction(result, expr->id())

to make sure that they're in tail position :)

[fschneider, 2011/06/30 13:13:36]

Done.

+      return;
     } else {
       AddInstruction(new(zone()) HCheckFunction(right, target));
-      instr = new(zone()) HInstanceOfKnownGlobal(left, target);
+      HInstanceOfKnownGlobal* result =
+          new(zone()) HInstanceOfKnownGlobal(left, target);
+      result->set_position(expr->position());
+      ast_context()->ReturnInstruction(result, expr->id());
+      return;
     }
   } else if (op == Token::IN) {
-    instr = new(zone()) HIn(left, right);
+    HIn* result = new(zone()) HIn(left, right);
+    result->set_position(expr->position());
+    ast_context()->ReturnInstruction(result, expr->id());
+    return;
   } else if (type_info.IsNonPrimitive()) {
     switch (op) {
       case Token::EQ:
@@ -5602,12 +5622,14 @@
         AddInstruction(HCheckInstanceType::NewIsSpecObject(left));
         AddInstruction(new(zone()) HCheckNonSmi(right));
         AddInstruction(HCheckInstanceType::NewIsSpecObject(right));
-        instr = new(zone()) HCompareObjectEq(left, right);
-        break;
+        HCompareObjectEqAndBranch* result =
+            new(zone()) HCompareObjectEqAndBranch(left, right);
+        result->set_position(expr->position());
+        ast_context()->ReturnInstruction(result, expr->id());
+        return;
       }
       default:
         return Bailout("Unsupported non-primitive compare");
-        break;
     }
   } else if (type_info.IsString() && oracle()->IsSymbolCompare(expr) &&
              (op == Token::EQ || op == Token::EQ_STRICT)) {
@@ -5615,27 +5637,57 @@
     AddInstruction(HCheckInstanceType::NewIsSymbol(left));
     AddInstruction(new(zone()) HCheckNonSmi(right));
     AddInstruction(HCheckInstanceType::NewIsSymbol(right));
-    instr = new(zone()) HCompareObjectEq(left, right);
+    HCompareObjectEqAndBranch* result =
+        new(zone()) HCompareObjectEqAndBranch(left, right);
+    result->set_position(expr->position());
+    ast_context()->ReturnInstruction(result, expr->id());
+    return;
   } else {
-    HCompare* compare = new(zone()) HCompare(left, right, op);
     Representation r = ToRepresentation(type_info);
-    compare->SetInputRepresentation(r);
-    instr = compare;
+    if (r.IsTagged()) {
+      HCompareGeneric* result = new(zone()) HCompareGeneric(left, right, op);
+      result->set_position(expr->position());
+      ast_context()->ReturnInstruction(result, expr->id());
+      return;
+    } else {
+      HCompareIDAndBranch* result =
+          new(zone()) HCompareIDAndBranch(left, right, op);
+      result->set_position(expr->position());
+      result->SetInputRepresentation(r);
+      ast_context()->ReturnInstruction(result, expr->id());
+      return;
+    }
   }
-  instr->set_position(expr->position());
-  ast_context()->ReturnInstruction(instr, expr->id());
 }
 
 
+void HGraphBuilder::MaterializeBoolean(HControlInstruction* instr,

[Kevin Millikin (Chromium), 2011/06/30 10:52:03]

I find this function weird.  It should never be called for a test context.  For
an effect context, there should be not push in the branch target blocks (and
therefore no pop in the join block).

[fschneider, 2011/06/30 13:13:36]

Removed.

+                                       int join_id) {
+  HBasicBlock* materialize_false = graph()->CreateBasicBlock();
+  HBasicBlock* materialize_true = graph()->CreateBasicBlock();
+  instr->SetSuccessorAt(0, materialize_true);
+  instr->SetSuccessorAt(1, materialize_false);
+  current_block()->Finish(instr);
+  set_current_block(materialize_false);
+  Push(graph()->GetConstantFalse());
+  set_current_block(materialize_true);
+  Push(graph()->GetConstantTrue());
+  HBasicBlock* join =
+    CreateJoin(materialize_false, materialize_true, join_id);
+  set_current_block(join);
+  ast_context()->ReturnValue(Pop());
+}
+
+
 void HGraphBuilder::VisitCompareToNull(CompareToNull* expr) {
   ASSERT(!HasStackOverflow());
   ASSERT(current_block() != NULL);
   ASSERT(current_block()->HasPredecessor());
   CHECK_ALIVE(VisitForValue(expr->expression()));
-
   HValue* value = Pop();
-  HIsNull* compare = new(zone()) HIsNull(value, expr->is_strict());
-  ast_context()->ReturnInstruction(compare, expr->id());
+  HIsNullAndBranch* instr =
+      new(zone()) HIsNullAndBranch(value, expr->is_strict());
+  ast_context()->ReturnInstruction(instr, expr->id());
 }
 
 
@@ -5668,7 +5720,7 @@
   ASSERT(call->arguments()->length() == 1);
   CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
   HValue* value = Pop();
-  HIsSmi* result = new(zone()) HIsSmi(value);
+  HIsSmiAndBranch* result = new(zone()) HIsSmiAndBranch(value);
   ast_context()->ReturnInstruction(result, call->id());
 }
 
@@ -5677,10 +5729,10 @@
   ASSERT(call->arguments()->length() == 1);
   CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
   HValue* value = Pop();
-  HHasInstanceType* result =
-      new(zone()) HHasInstanceType(value,
-                                   FIRST_SPEC_OBJECT_TYPE,
-                                   LAST_SPEC_OBJECT_TYPE);
+  HHasInstanceTypeAndBranch* result =
+      new(zone()) HHasInstanceTypeAndBranch(value,
+                                            FIRST_SPEC_OBJECT_TYPE,
+                                            LAST_SPEC_OBJECT_TYPE);
   ast_context()->ReturnInstruction(result, call->id());
 }
 
@@ -5689,8 +5741,8 @@
   ASSERT(call->arguments()->length() == 1);
   CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
   HValue* value = Pop();
-  HHasInstanceType* result =
-      new(zone()) HHasInstanceType(value, JS_FUNCTION_TYPE);
+  HHasInstanceTypeAndBranch* result =
+      new(zone()) HHasInstanceTypeAndBranch(value, JS_FUNCTION_TYPE);
   ast_context()->ReturnInstruction(result, call->id());
 }
 
@@ -5699,7 +5751,8 @@
   ASSERT(call->arguments()->length() == 1);
   CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
   HValue* value = Pop();
-  HHasCachedArrayIndex* result = new(zone()) HHasCachedArrayIndex(value);
+  HHasCachedArrayIndexAndBranch* result =
+      new(zone()) HHasCachedArrayIndexAndBranch(value);
   ast_context()->ReturnInstruction(result, call->id());
 }
 
@@ -5708,7 +5761,8 @@
   ASSERT(call->arguments()->length() == 1);
   CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
   HValue* value = Pop();
-  HHasInstanceType* result = new(zone()) HHasInstanceType(value, JS_ARRAY_TYPE);
+  HHasInstanceTypeAndBranch* result =
+      new(zone()) HHasInstanceTypeAndBranch(value, JS_ARRAY_TYPE);
   ast_context()->ReturnInstruction(result, call->id());
 }
 
@@ -5717,8 +5771,8 @@
   ASSERT(call->arguments()->length() == 1);
   CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
   HValue* value = Pop();
-  HHasInstanceType* result =
-      new(zone()) HHasInstanceType(value, JS_REGEXP_TYPE);
+  HHasInstanceTypeAndBranch* result =
+      new(zone()) HHasInstanceTypeAndBranch(value, JS_REGEXP_TYPE);
   ast_context()->ReturnInstruction(result, call->id());
 }
 
@@ -5727,8 +5781,8 @@
   ASSERT(call->arguments()->length() == 1);
   CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
   HValue* value = Pop();
-  HIsObject* test = new(zone()) HIsObject(value);
-  ast_context()->ReturnInstruction(test, call->id());
+  HIsObjectAndBranch* result = new(zone()) HIsObjectAndBranch(value);
+  ast_context()->ReturnInstruction(result, call->id());
 }
 
 
@@ -5741,8 +5795,9 @@
   ASSERT(call->arguments()->length() == 1);
   CHECK_ALIVE(VisitForValue(call->arguments()->at(0)));
   HValue* value = Pop();
-  ast_context()->ReturnInstruction(new(zone()) HIsUndetectable(value),
-                                   call->id());
+  HIsUndetectableAndBranch* result =
+      new(zone()) HIsUndetectableAndBranch(value);
+  ast_context()->ReturnInstruction(result, call->id());
 }
 
 
@@ -5762,7 +5817,8 @@
     // false from %_IsConstructCall().
     ast_context()->ReturnValue(graph()->GetConstantFalse());
   } else {
-    ast_context()->ReturnInstruction(new(zone()) HIsConstructCall, call->id());
+    ast_context()->ReturnInstruction(new(zone()) HIsConstructCallAndBranch,
+                                     call->id());
   }
 }
 
@@ -5861,7 +5917,8 @@
   CHECK_ALIVE(VisitForValue(call->arguments()->at(1)));
   HValue* right = Pop();
   HValue* left = Pop();
-  HCompareObjectEq* result = new(zone()) HCompareObjectEq(left, right);
+  HCompareObjectEqAndBranch* result =
+      new(zone()) HCompareObjectEqAndBranch(left, right);
   ast_context()->ReturnInstruction(result, call->id());
 }