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

runtime/vm/flow_graph_builder.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/flow_graph_builder.h"
 
 #include "vm/ast_printer.h"
 #include "vm/bit_vector.h"
 #include "vm/code_descriptors.h"
 #include "vm/dart_entry.h"
@@ skipping 240 matching lines @@
 }
 
 
 // Loads context saved in 'context_variable' into the current context.
 void EffectGraphVisitor::BuildLoadContext(const LocalVariable& variable) {
   Value* load_saved_context = Bind(BuildLoadLocal(variable));
   Do(new StoreContextComp(load_saved_context));
 }
 
 
-// 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(),
                                        owner()->try_index(),
                                        value));
   }
   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(),
                                         value,
                                         constant_true,
                                         Token::kEQ_STRICT);
   AddInstruction(branch);
   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);
 }
 
 
 // <Statement> ::= Return { value:                <Expression>
 //                          inlined_finally_list: <InlinedFinally>* }
@@ skipping 513 matching lines @@
     return;
   }
 
   if ((node->kind() == Token::kEQ) || (node->kind() == Token::kNE)) {
     ValueGraphVisitor for_left_value(owner(), temp_index());
     node->left()->Visit(&for_left_value);
     Append(for_left_value);
     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;
   }
 
   ValueGraphVisitor for_left_value(owner(), temp_index());
   node->left()->Visit(&for_left_value);
   Append(for_left_value);
   ValueGraphVisitor for_right_value(owner(), temp_index());
   node->right()->Visit(&for_right_value);
   Append(for_right_value);
@@ skipping 1824 matching lines @@
   char* chars = reinterpret_cast<char*>(
       Isolate::Current()->current_zone()->Allocate(len));
   OS::SNPrint(chars, len, kFormat, function_name, reason);
   const Error& error = Error::Handle(
       LanguageError::New(String::Handle(String::New(chars))));
   Isolate::Current()->long_jump_base()->Jump(1, error);
 }
 
 
 }  // namespace dart