Improve code for !identical(a, b):

runtime/vm/flow_graph_optimizer.cc

Index: runtime/vm/flow_graph_optimizer.cc
===================================================================
--- runtime/vm/flow_graph_optimizer.cc	(revision 20429)
+++ runtime/vm/flow_graph_optimizer.cc	(working copy)
@@ -2110,6 +2110,32 @@
 }
 
 
+
+// Check if this StrictCompare can be merged with its input:
+// x = (a === b); y = (x !== true) -> y = a !== b.
+// This is a common occurence for !identical(a, b).
+void FlowGraphOptimizer::HandleStrictCompare(StrictCompareInstr* comp,
+                                             BranchInstr* instr) {
+  if ((comp->kind() != Token::kNE_STRICT) ||
+      !comp->right()->BindsToConstant() ||
+      (comp->right()->BoundConstant().raw() != Bool::True().raw())) {
+    return;
+  }
+  StrictCompareInstr* left = comp->left()->definition()->AsStrictCompare();
+  if (left == NULL) {
+    return;
+  }
+  Token::Kind reverted_kind = (left->kind() == Token::kNE_STRICT) ?
+      Token::kEQ_STRICT : Token::kNE_STRICT;

[Kevin Millikin (Google), 2013/03/25 09:58:08]

We should probably have a generic way to do this, either a function on the Token
class or a function on the StrictCompareInstr class.

There is a static function in this file, NegateComparison, that you can use if
you extend it to handle strict comparisons.

[srdjan, 2013/03/25 21:12:11]

Extended NegateComparison and moved that functionality to Token class.

+  StrictCompareInstr* reverted_comp =
+      new StrictCompareInstr(reverted_kind,
+                             left->left()->Copy(),
+                             left->right()->Copy());
+  instr->ReplaceWith(reverted_comp, current_iterator());

[Kevin Millikin (Google), 2013/03/25 09:58:08]

I still think it's nuts that BranchInstr::ReplaceWith takes a comparison and
doesn't replace the branch with anything.  It only exists so it can be used in a
template function.

Just call instr->SetComparison directly here for clarity.

[srdjan, 2013/03/25 21:12:11]

Done.

+  left->RemoveFromGraph();

[Kevin Millikin (Google), 2013/03/25 09:58:08]

You can't remove this if it has uses, even environment ones.

[srdjan, 2013/03/25 21:12:11]

Thanks. Adding tests to catch that. Would it be useful if we could assert that
in RemoveFromGraph? (right now we can't).

+}
+
+
 void FlowGraphOptimizer::VisitBranch(BranchInstr* instr) {
   ComparisonInstr* comparison = instr->comparison();
   if (comparison->IsRelationalOp()) {
@@ -2118,7 +2144,7 @@
     HandleEqualityCompare(comparison->AsEqualityCompare(), instr);
   } else {
     ASSERT(comparison->IsStrictCompare());
-    // Nothing to do.
+    HandleStrictCompare(comparison->AsStrictCompare(), instr);

[Kevin Millikin (Google), 2013/03/25 09:58:08]

I feel that this belongs better in the canonicalize pass rather than in this
pass which is doing rewriting based on types.  

There is a TODO from fschneider in StrictCompareInstr::Canonicalize to handle
exactly this pattern.  Unfortunately, it's a bit messy because you want to
rewrite two instructions.  You could try:

1. The existing pattern in StrictCompareInstr::Canonicalize, which seems a bit
warty: return the first comparison as the result of canonicalizing the second,
peek at the return value in BranchInstr::Comparison to see if the first
comparison should be removed from the graph and the second comparison replaced
with the first in the BranchInstr.  Here you can directly use the first
comparison but swap the successors of the branch.

2. When canonicalizing the first comparison, possibly eliminate it and rewrite
the second.

3. When canonicalizing the second comparison, rewrite it and  possibly eliminate
the first.

[srdjan, 2013/03/25 21:12:11]

I agree, moved.

   }
 }