VM: Fix bug in canonicalization of identical in the optimizing compiler.

runtime/vm/intermediate_language.cc

 // Copyright (c) 2013, 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/intermediate_language.h"
 
 #include "vm/bigint_operations.h"
 #include "vm/bit_vector.h"
 #include "vm/dart_entry.h"
 #include "vm/flow_graph_allocator.h"
@@ skipping 1497 matching lines @@
 }
 
 
 Definition* UnboxUint32x4Instr::Canonicalize(FlowGraph* flow_graph) {
   // Fold away UnboxUint32x4(BoxUint32x4(v)).
   BoxUint32x4Instr* defn = value()->definition()->AsBoxUint32x4();
   return (defn != NULL) ? defn->value()->definition() : this;
 }
 
 
+static Definition* CanonicalizeStrictCompare(StrictCompareInstr* compare,
+                                             bool* negated) {
+  ASSERT(!*negated);

[srdjan, 2013/10/15 15:10:16]

It seems safer to set *negated to false here than to expect the caller to
initialize it to false.

[Florian Schneider, 2013/10/15 15:15:03]

Done.

+  if (!compare->right()->BindsToConstant()) {
+    return compare;
+  }
+  const Object& right_constant = compare->right()->BoundConstant();
+  Definition* left_defn = compare->left()->definition();
+
+  // TODO(fschneider): Handle other cases: e === false and e !== true/false.
+  // Handles e === true.
+  if ((compare->kind() == Token::kEQ_STRICT) &&
+      (right_constant.raw() == Bool::True().raw()) &&
+      (compare->left()->Type()->ToCid() == kBoolCid)) {
+    // Return left subexpression as the replacement for this instruction.
+    return left_defn;
+  }
+  // x = (a === b);  y = x !== true; -> y = a !== b.
+  // In order to merge two strict compares, 'left_strict' must have only one
+  // use. Do not check left's cid as it is required to be a strict compare.
+  StrictCompareInstr* left_strict = left_defn->AsStrictCompare();
+  if ((compare->kind() == Token::kNE_STRICT) &&
+      (right_constant.raw() == Bool::True().raw()) &&
+      (left_strict != NULL) &&
+      (left_strict->HasOnlyUse(compare->left()))) {
+    *negated = true;
+    return left_strict;
+  }
+  return compare;
+}
+
+
 Instruction* BranchInstr::Canonicalize(FlowGraph* flow_graph) {
   // Only handle strict-compares.
   if (comparison()->IsStrictCompare()) {
-    Definition* replacement = comparison()->Canonicalize(flow_graph);
-    if ((replacement == comparison()) || (replacement == NULL)) {
+    bool negated = false;
+    Definition* replacement =
+        CanonicalizeStrictCompare(comparison()->AsStrictCompare(), &negated);
+    if (replacement == comparison()) {
       return this;
     }
     ComparisonInstr* comp = replacement->AsComparison();
     if ((comp == NULL) || comp->CanDeoptimize()) {
       return this;
     }
 
-    // Check that comparison is not serving as a pending deoptimization target
-    // for conversions.
+    // Assert that the comparison is not serving as a pending deoptimization
+    // target for conversions.
     for (intptr_t i = 0; i < comp->InputCount(); i++) {
       if (comp->RequiredInputRepresentation(i) !=
           comp->InputAt(i)->definition()->representation()) {
         return this;
       }
     }
 
     // Replace the comparison if the replacement is used at this branch,
     // and has exactly one use.
     Value* use = comp->input_use_list();
     if ((use->instruction() == this) && comp->HasOnlyUse(use)) {
+      if (negated) {
+        comp->NegateComparison();
+      }
       RemoveEnvironment();
       flow_graph->CopyDeoptTarget(this, comp);
 
       comp->RemoveFromGraph();
       SetComparison(comp);
       if (FLAG_trace_optimization) {
         OS::Print("Merging comparison v%" Pd "\n", comp->ssa_temp_index());
       }
       // Clear the comparison's temp index and ssa temp index since the
       // value of the comparison is not used outside the branch anymore.
       ASSERT(comp->input_use_list() == NULL);
       comp->ClearSSATempIndex();
       comp->ClearTempIndex();
     }
   }
   return this;
 }
 
 
 Definition* StrictCompareInstr::Canonicalize(FlowGraph* flow_graph) {
-  if (!right()->BindsToConstant()) {
-    return this;
+  bool negated = false;
+  Definition* replacement = CanonicalizeStrictCompare(this, &negated);
+  if (negated && replacement->IsComparison()) {
+    ASSERT(replacement != this);
+    replacement->AsComparison()->NegateComparison();
   }
-  const Object& right_constant = right()->BoundConstant();
-  Definition* left_defn = left()->definition();
-  // TODO(fschneider): Handle other cases: e === false and e !== true/false.
-  // Handles e === true.
-  if ((kind() == Token::kEQ_STRICT) &&
-      (right_constant.raw() == Bool::True().raw()) &&
-      (left()->Type()->ToCid() == kBoolCid)) {
-    // Return left subexpression as the replacement for this instruction.
-    return left_defn;
-  }
-  // x = (a === b);  y = x !== true; -> y = a !== b.
-  // In order to merge two strict comares, 'left_strict' must have only one use.
-  // Do not check left's cid as it is required to be a strict compare.
-  StrictCompareInstr* left_strict = left_defn->AsStrictCompare();
-  if ((kind() == Token::kNE_STRICT) &&
-      (right_constant.raw() == Bool::True().raw()) &&
-      (left_strict != NULL) &&
-      (left_strict->HasOnlyUse(left()))) {
-    left_strict->set_kind(Token::NegateComparison(left_strict->kind()));
-    return left_strict;
-  }
-
-  return this;
+  return replacement;
 }
 
 
 Instruction* CheckClassInstr::Canonicalize(FlowGraph* flow_graph) {
   // TODO(vegorov): Replace class checks with null checks when ToNullableCid
   // matches.
 
   const intptr_t value_cid = value()->Type()->ToCid();
   if (value_cid == kDynamicCid) {
     return this;
@@ skipping 1101 matching lines @@
       return kCosRuntimeEntry;
     default:
       UNREACHABLE();
   }
   return kSinRuntimeEntry;
 }
 
 #undef __
 
 }  // namespace dart