Fix misoptimization of 'is' test

runtime/vm/optimizer.cc

Index: runtime/vm/optimizer.cc
diff --git a/runtime/vm/optimizer.cc b/runtime/vm/optimizer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..94325ccfc12b132252ec4e7cdee92cd095fcf747
--- /dev/null
+++ b/runtime/vm/optimizer.cc
@@ -0,0 +1,100 @@
+// Copyright (c) 2017, 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/optimizer.h"
+
+#include "vm/intermediate_language.h"
+#include "vm/object.h"
+
+namespace dart {
+
+static bool CidTestResultsContains(const ZoneGrowableArray<intptr_t>& results,
+                                   intptr_t test_cid) {
+  for (intptr_t i = 0; i < results.length(); i += 2) {
+    if (results[i] == test_cid) return true;
+  }
+  return false;
+}
+
+
+static void TryAddTest(ZoneGrowableArray<intptr_t>* results,
+                       intptr_t test_cid,
+                       bool result) {
+  if (!CidTestResultsContains(*results, test_cid)) {
+    results->Add(test_cid);
+    results->Add(result);
+  }
+}
+
+
+// Used when we only need the positive result because we return false by
+// default.
+static void PurgeNegativeTestCidsEntries(ZoneGrowableArray<intptr_t>* results) {
+  int dest = 0;
+  for (intptr_t i = 0; i < results->length(); i += 2) {

[Vyacheslav Egorov (Google), 2017/05/23 16:00:31]

It feels like you can just start with i = dest = 1 because first entry is always
a smi entry.

[erikcorry, 2017/05/24 13:37:16]

Done.

+    // We can't purge the Smi entry at the beginning since it is used in the
+    // Smi check before the Cid is loaded.
+    if (results->At(i + 1) || results->At(i) == kSmiCid) {

[Vyacheslav Egorov (Google), 2017/05/23 16:00:31]

At(i + 1) != 0 - we don't use implicit intptr_t -> boolean coercion.

[erikcorry, 2017/05/24 13:37:16]

Done.

+      (*results)[dest++] = results->At(i);
+      (*results)[dest++] = results->At(i + 1);
+    }
+  }
+  results->SetLength(dest);
+}
+
+
+// Tries to add cid tests to 'results' so that no deoptimization is

[Vyacheslav Egorov (Google), 2017/05/23 16:00:32]

Maybe move this comment to the header file?

[erikcorry, 2017/05/24 13:37:16]

Done.

+// necessary for common number-related type tests.  Unconditionally adds an
+// entry for the Smi type to the start of the array.
+// TODO(srdjan): Do also for other than numeric types.

[Vyacheslav Egorov (Google), 2017/05/23 16:00:32]

remove TODO(srdjan)

[erikcorry, 2017/05/23 18:24:40]

Do you mean I should remove the whole line?

Is this because we no longer want to do this change (it's still not done)?

[Vyacheslav Egorov (Google), 2017/05/23 18:33:15]

I don't think srdjan is gonna do it. Also it is already done - you handle all
numeric types in this function.

[erikcorry, 2017/05/23 18:35:09]

OK I'll remove it, but the TODO was to handle non-numeric types.

[Vyacheslav Egorov (Google), 2017/05/23 18:36:13]

Ooops. I misread it - but anyway if you want to keep it file a bug and rewrite
TODO(srdjan) to TODO(dartbug.com/bugnumber)

[erikcorry, 2017/05/24 13:37:16]

Gone

+bool Optimizer::TryExpandTestCidsResult(ZoneGrowableArray<intptr_t>* results,

[Vyacheslav Egorov (Google), 2017/05/23 16:00:32]

I wonder if this should be using a better type than just intptr_t: 

class CidTest {
  intptr_t cid;
  bool result;
};

ZoneGrowableArray<CidTest> tests

[erikcorry, 2017/05/24 13:37:16]

It should, but I'd just like to fix the bug.  Slightly longer term we probably
want to use CidRange here.

+                                        const AbstractType& type,
+                                        bool int_type_only) {
+  ASSERT(results->length() >= 2);  // At least on entry.
+  const ClassTable& class_table = *Isolate::Current()->class_table();
+  if ((*results)[0] != kSmiCid) {
+    const Class& cls = Class::Handle(class_table.At(kSmiCid));
+    const Class& type_class = Class::Handle(type.type_class());
+    const bool smi_is_subtype =
+        cls.IsSubtypeOf(Object::null_type_arguments(), type_class,
+                        Object::null_type_arguments(), NULL, NULL, Heap::kOld);
+    results->Add((*results)[results->length() - 2]);
+    results->Add((*results)[results->length() - 2]);
+    for (intptr_t i = results->length() - 3; i > 1; --i) {
+      (*results)[i] = (*results)[i - 2];
+    }
+    (*results)[0] = kSmiCid;
+    (*results)[1] = smi_is_subtype;
+  }
+
+  ASSERT(type.IsInstantiated() && !type.IsMalformedOrMalbounded());
+  ASSERT(results->length() >= 2);
+  if (type.IsSmiType() && !int_type_only) {
+    ASSERT((*results)[0] == kSmiCid);
+    PurgeNegativeTestCidsEntries(results);
+    return false;
+  } else if (type.IsIntType()) {
+    ASSERT((*results)[0] == kSmiCid);
+    TryAddTest(results, kMintCid, true);
+    TryAddTest(results, kBigintCid, true);
+    // Cannot deoptimize since all tests returning true have been added.
+    PurgeNegativeTestCidsEntries(results);
+    return false;
+  } else if (type.IsNumberType() && !int_type_only) {
+    ASSERT((*results)[0] == kSmiCid);
+    TryAddTest(results, kMintCid, true);
+    TryAddTest(results, kBigintCid, true);
+    TryAddTest(results, kDoubleCid, true);
+    PurgeNegativeTestCidsEntries(results);
+    return false;
+  } else if (type.IsDoubleType() && !int_type_only) {
+    ASSERT((*results)[0] == kSmiCid);
+    TryAddTest(results, kDoubleCid, true);
+    PurgeNegativeTestCidsEntries(results);
+    return false;
+  }
+  return true;  // May deoptimize since we have not identified all 'true' tests.
+}
+
+}  // namespace dart