Apply Kevin's suggestions, make branch-compare generation more robust

runtime/vm/flow_graph_builder.cc

Index: runtime/vm/flow_graph_builder.cc
===================================================================
--- runtime/vm/flow_graph_builder.cc	(revision 9777)
+++ runtime/vm/flow_graph_builder.cc	(working copy)
@@ -258,50 +258,6 @@
 }
 
 
-// TODO(srdjan): This code is clumsy. Try to skip allocation of ComparisonComp
-// and generate the right BranchInstr directly.
-// Replaces instruction patterns:
-//   t0 <- Comparison(kind, t0, t1)
-//   t1 <- #true
-//   Branch if t0 === t1 goto (true, false)
-// With:
-//   Branch if t0 kind t1 goto (true, false)
-// Returns false if fusing is not possible, e.g:
-//   t0 <- LoadLocal
-//   t1 <- #true
-//   Branch if t0 === t1 goto (true, false)
-static bool TryFuseBranchInstr(BranchInstr* branch) {
-  UseVal* use = branch->left()->AsUse();
-  if (use == NULL) return false;
-  BindInstr* compare_instr = use->definition()->AsBind();
-  if (compare_instr == NULL) return false;
-  ComparisonComp* compare = compare_instr->computation()->AsComparison();
-  Token::Kind kind;
-  if (compare == NULL) {
-    // Check if there is a BooleanNegate in between Comparison and Branch.
-    BooleanNegateComp* neg = compare_instr->computation()->AsBooleanNegate();
-    if (neg == NULL) return false;
-    if (compare_instr->previous() == NULL) return false;
-    compare_instr = compare_instr->previous()->AsBind();
-    ASSERT(compare_instr != NULL);
-    compare = compare_instr->computation()->AsComparison();
-    if (compare == NULL) return false;
-    // Negation can be handled only in connection with EqualityCompare.
-    if (!compare->IsEqualityCompare()) return false;
-    kind = Token::kNE;
-  } else {
-    kind = compare->kind();
-  }
-  branch->set_kind(kind);
-  branch->SetInputAt(0, compare->InputAt(0));
-  branch->SetInputAt(1, compare->InputAt(1));
-  // Remove elminated nodes.
-  branch->set_previous(compare_instr->previous());
-  compare_instr->previous()->set_next(branch);
-  return true;
-}
-
-
 void TestGraphVisitor::ReturnValue(Value* value) {
   if (FLAG_enable_type_checks) {
     value = Bind(new AssertBooleanComp(condition_token_pos(),
@@ -319,10 +275,54 @@
   CloseFragment();
   true_successor_address_ = branch->true_successor_address();
   false_successor_address_ = branch->false_successor_address();
-  TryFuseBranchInstr(branch);
 }
 
 
+void TestGraphVisitor::MergeBranchWithComparison(ComparisonComp* comp) {
+  ASSERT(!FLAG_enable_type_checks);
+  BranchInstr* branch = new BranchInstr(condition_token_pos(),
+                                        owner()->try_index(),
+                                        comp->left(),
+                                        comp->right(),
+                                        comp->kind());
+  AddInstruction(branch);
+  CloseFragment();
+  true_successor_address_ = branch->true_successor_address();
+  false_successor_address_ = branch->false_successor_address();
+}
+
+
+void TestGraphVisitor::MergeBranchWithNegate(BooleanNegateComp* comp) {
+  ASSERT(!FLAG_enable_type_checks);
+  const Bool& bool_true = Bool::ZoneHandle(Bool::True());
+  Value* constant_true = Bind(new ConstantVal(bool_true));
+  BranchInstr* branch = new BranchInstr(condition_token_pos(),
+                                        owner()->try_index(),
+                                        comp->value(),
+                                        constant_true,
+                                        Token::kNE_STRICT);
+  AddInstruction(branch);
+  CloseFragment();
+  true_successor_address_ = branch->true_successor_address();
+  false_successor_address_ = branch->false_successor_address();
+}
+
+
+void TestGraphVisitor::ReturnComputation(Computation* computation) {
+  if (!FLAG_enable_type_checks) {
+    if (computation->AsComparison() != NULL) {
+      MergeBranchWithComparison(computation->AsComparison());
+      return;
+    }
+    if (computation->IsBooleanNegate()) {
+      MergeBranchWithNegate(computation->AsBooleanNegate());
+      return;
+    }
+  }
+  ReturnValue(Bind(computation));
+}
+
+
 void EffectGraphVisitor::Bailout(const char* reason) {
   owner()->Bailout(reason);
 }
@@ -853,20 +853,24 @@
     ValueGraphVisitor for_right_value(owner(), temp_index());
     node->right()->Visit(&for_right_value);
     Append(for_right_value);
-    EqualityCompareComp* comp = new EqualityCompareComp(
-        node->token_pos(), owner()->try_index(),
-        for_left_value.value(), for_right_value.value());
-    if (node->kind() == Token::kEQ) {
+    if (FLAG_enable_type_checks) {
+      EqualityCompareComp* comp = new EqualityCompareComp(
+          node->token_pos(), owner()->try_index(),
+          Token::kEQ, for_left_value.value(), for_right_value.value());
+      if (node->kind() == Token::kEQ) {
+        ReturnComputation(comp);
+      } else {
+        Value* eq_result = Bind(comp);
+        eq_result = Bind(new AssertBooleanComp(node->token_pos(),
+                                               owner()->try_index(),
+                                               eq_result));
+        ReturnComputation(new BooleanNegateComp(eq_result));
+      }
+    } else {
+      EqualityCompareComp* comp = new EqualityCompareComp(
+          node->token_pos(), owner()->try_index(),
+          node->kind(), for_left_value.value(), for_right_value.value());
       ReturnComputation(comp);
-    } else {
-      Value* eq_result = Bind(comp);
-      if (FLAG_enable_type_checks) {
-        eq_result =
-            Bind(new AssertBooleanComp(node->token_pos(),
-                                       owner()->try_index(),
-                                       eq_result));
-      }
-      ReturnComputation(new BooleanNegateComp(eq_result));
     }
     return;
   }