From dff13bef8de104d33b04820136da2d80f3c835d7 Mon Sep 17 00:00:00 2001
From: Florian Schneider <fschneider@google.com>
Date: Thu, 3 Dec 2015 21:14:35 +0100
Subject: [PATCH] VM: Fix bug in type propagation at conditionals.

Propagating type/cid at conditional branches is not possible because
it may cause invalid code motion.

For this to be safe we need to explicitly represent the dependency
between checks eliminated in a branch and the condition that constrains the type/cid.

BUG=
R=vegorov@google.com

Review URL: https://codereview.chromium.org/1491373005 .
---
 runtime/vm/flow_graph_type_propagator.cc     | 57 --------------------
 runtime/vm/flow_graph_type_propagator.h      |  1 -
 tests/language/vm/type_propagation_test.dart | 56 +++++++++++++++++++
 3 files changed, 56 insertions(+), 58 deletions(-)
 create mode 100644 tests/language/vm/type_propagation_test.dart

diff --git a/runtime/vm/flow_graph_type_propagator.cc b/runtime/vm/flow_graph_type_propagator.cc
index 986615f1b66..11df98a1d3f 100644
--- a/runtime/vm/flow_graph_type_propagator.cc
+++ b/runtime/vm/flow_graph_type_propagator.cc
@@ -149,8 +149,6 @@ void FlowGraphTypePropagator::PropagateRecursive(BlockEntryInstr* block) {
     }
   }
 
-  HandleBranchOnStrictCompare(block);
-
   for (intptr_t i = 0; i < block->dominated_blocks().length(); ++i) {
     PropagateRecursive(block->dominated_blocks()[i]);
   }
@@ -159,61 +157,6 @@ void FlowGraphTypePropagator::PropagateRecursive(BlockEntryInstr* block) {
 }
 
 
-void FlowGraphTypePropagator::HandleBranchOnStrictCompare(
-    BlockEntryInstr* block) {
-  BranchInstr* branch = block->last_instruction()->AsBranch();
-  if (branch == NULL) {
-    return;
-  }
-
-  StrictCompareInstr* compare = branch->comparison()->AsStrictCompare();
-  if ((compare == NULL) || !compare->right()->BindsToConstant()) {
-    return;
-  }
-
-  const intptr_t rollback_point = rollback_.length();
-
-  Definition* defn = compare->left()->definition();
-  const Object& right = compare->right()->BoundConstant();
-  intptr_t cid = right.GetClassId();
-
-  if (defn->IsLoadClassId() && right.IsSmi()) {
-    defn = defn->AsLoadClassId()->object()->definition();
-    cid = Smi::Cast(right).Value();
-  }
-
-  if (!CheckClassInstr::IsImmutableClassId(cid)) {
-    if ((cid == kOneByteStringCid) || (cid == kTwoByteStringCid)) {
-      SetTypeOf(defn, ZoneCompileType::Wrap(CompileType::String()));
-      PropagateRecursive((compare->kind() == Token::kEQ_STRICT) ?
-          branch->true_successor() : branch->false_successor());
-      RollbackTo(rollback_point);
-    }
-    return;
-  }
-
-  if (compare->kind() == Token::kEQ_STRICT) {
-    if (cid == kNullCid) {
-      branch->set_constrained_type(MarkNonNullable(defn));
-      PropagateRecursive(branch->false_successor());
-    }
-
-    SetCid(defn, cid);
-    PropagateRecursive(branch->true_successor());
-  } else if (compare->kind() == Token::kNE_STRICT) {
-    if (cid == kNullCid) {
-      branch->set_constrained_type(MarkNonNullable(defn));
-      PropagateRecursive(branch->true_successor());
-    }
-
-    SetCid(defn, cid);
-    PropagateRecursive(branch->false_successor());
-  }
-
-  RollbackTo(rollback_point);
-}
-
-
 void FlowGraphTypePropagator::RollbackTo(intptr_t rollback_point) {
   for (intptr_t i = rollback_.length() - 1; i >= rollback_point; i--) {
     types_[rollback_[i].index()] = rollback_[i].type();
diff --git a/runtime/vm/flow_graph_type_propagator.h b/runtime/vm/flow_graph_type_propagator.h
index 4b1baf4e1ff..3c1091769e4 100644
--- a/runtime/vm/flow_graph_type_propagator.h
+++ b/runtime/vm/flow_graph_type_propagator.h
@@ -20,7 +20,6 @@ class FlowGraphTypePropagator : public FlowGraphVisitor {
   void Propagate();
 
   void PropagateRecursive(BlockEntryInstr* block);
-  void HandleBranchOnStrictCompare(BlockEntryInstr* block);
 
   void RollbackTo(intptr_t rollback_point);
 
diff --git a/tests/language/vm/type_propagation_test.dart b/tests/language/vm/type_propagation_test.dart
new file mode 100644
index 00000000000..935c0964880
--- /dev/null
+++ b/tests/language/vm/type_propagation_test.dart
@@ -0,0 +1,56 @@
+// Copyright (c) 2015, 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.
+// VMOptions=--optimization-counter-threshold=1000 --max-polymorphic-checks=1
+
+// Test correct loop invariant code motion and type propagation from is-checks
+// and null-comparisons. 
+
+class B {
+  var b;
+  B(this.b);
+}
+
+class C {
+  final f0;
+
+  final a;
+  C() : a = new B(0);
+}
+
+foo(x) {
+  for (var i = 0; i < 10; i++) {
+    i + i;
+    i + i;
+    if (x is C) {
+      x.a.b < 0;
+    }
+  }
+}
+
+class Y { var f = null; }
+
+bar(y) {
+  var x = y.f;
+  for (var i = 0; i < 10; i++) {
+    if (x != null) {
+      x.a.b < 0;
+    }
+  }
+}
+
+
+main () {
+  var o = new Y();
+  o.f = new C();
+  bar(o);
+  o.f = null;
+  bar(o);
+
+  for (var i = 0; i < 1000; i++) bar(o);
+
+  foo(new C());
+  foo(0);
+
+  for (var i = 0; i < 1000; i++) foo(0);
+}