Faster IC stubs by specializing them for Binary Smi operations

runtime/vm/object.cc

Index: runtime/vm/object.cc
===================================================================
--- runtime/vm/object.cc	(revision 39307)
+++ runtime/vm/object.cc	(working copy)
@@ -6634,7 +6634,7 @@
   const Array& saved_icd = Array::Handle(isolate, ic_data_array());
   if (saved_icd.Length() == 0) {
     deopt_id_to_ic_data->Clear();
-    return;;
+    return;
   }
   ICData& icd = ICData::Handle();
   icd ^= saved_icd.At(saved_icd.Length() - 1);
@@ -6646,6 +6646,11 @@
   for (intptr_t i = 0; i < saved_icd.Length(); i++) {
     ICData& icd = ICData::ZoneHandle(isolate);
     icd ^= saved_icd.At(i);
+    // Remove unused entries that have been added at creation time (e.g.,
+    // optimistic Smi/Smi assumption).
+    if (icd.HasUnusedEntries()) {
+      icd = icd.FilterUnusedEntries();
+    }
     (*deopt_id_to_ic_data)[icd.deopt_id()] = &icd;
   }
 }
@@ -11348,7 +11353,7 @@
     AddCheck(class_ids, target);
     return;
   }
-  ASSERT(NumArgsTested() >= 0);
+  ASSERT(NumArgsTested() == 0);
   // Can add only once.
   const intptr_t old_num = NumberOfChecks();
   ASSERT(old_num == 0);
@@ -11360,6 +11365,8 @@
   intptr_t data_pos = old_num * TestEntryLength();
   ASSERT(!target.IsNull());
   data.SetAt(data_pos++, target);
+  // Set count to 0 as this is called during compilation, before the
+  // call has been executed.
   const Smi& value = Smi::Handle(Smi::New(0));
   data.SetAt(data_pos, value);
 }
@@ -11368,6 +11375,12 @@
 void ICData::AddCheck(const GrowableArray<intptr_t>& class_ids,
                       const Function& target) const {
   ASSERT(!target.IsNull());
+  if (target.name() != target_name()) {
+    OS::Print("%s vs %s\n",
+        String::Handle(target.name()).ToCString(),

[Cutch, 2014/08/15 21:23:28]

Should this be an ASSERT?

[srdjan, 2014/08/15 21:49:00]

This is gone, forgot to upload after removing it. Sorry. The assert is below,
this was debugging code as I could not print from LLDB

+        String::Handle(target_name()).ToCString());
+  }
+  ASSERT(target.name() == target_name());
   DEBUG_ASSERT(!HasCheck(class_ids));
   ASSERT(NumArgsTested() > 1);  // Otherwise use 'AddReceiverCheck'.
   ASSERT(class_ids.length() == NumArgsTested());
@@ -11666,6 +11679,46 @@
 }
 
 
+bool ICData::HasUnusedEntries() const {
+  const intptr_t len = NumberOfChecks();
+  for (intptr_t i = 0; i < len; i++) {
+    if (GetCountAt(i) == 0) {
+      // Do not mistake unoptimized static call ICData for unused.
+      // See ICData::AddTarget.
+      // TODO(srdjan): Make this test more robust.

[Cutch, 2014/08/15 21:23:28]

What about factoring this check into:

bool IsUnused(intptr_t i) const;

And then using it below in FilterUnusedEntries for consistency.

[srdjan, 2014/08/15 21:49:00]

FilterUnusedEntries currently always creates a new ICData, I would like to do
that only if needed, i.e., I need first one pass to determine if allocation and
copying is needed at all.

[srdjan, 2014/08/18 20:28:40]

Discussed offline, misunderstood the comment.
Done.

+      if (NumArgsTested() > 0) {
+        const intptr_t cid = GetReceiverClassIdAt(i);
+        if (cid != kObjectCid) {
+          return true;
+        }
+      }
+    }
+  }
+  return false;
+}
+
+
+RawICData* ICData::FilterUnusedEntries() const {
+  ICData& result = ICData::Handle(ICData::New(
+      Function::Handle(owner()),
+      String::Handle(target_name()),
+      Array::Handle(arguments_descriptor()),
+      deopt_id(),
+      NumArgsTested()));
+  const intptr_t len = NumberOfChecks();
+  GrowableArray<intptr_t> class_ids;
+  Function& target = Function::Handle();
+  for (intptr_t i = 0; i < len; i++) {
+    const intptr_t count = GetCountAt(i);
+    if (count <= 0) continue;

[zra, 2014/08/15 21:45:08]

Above, GetCountAt(i) is checked for equality with zero. Why is it different
here? Also, please use curly braces around continue;

Above, there are also a checks for NumArgsTest() > 0 and on the cid. Why don't
you need to do those here, too? Couldn't you be filtering too many things?

[srdjan, 2014/08/18 20:28:40]

Done using Cutch's suggestion.

+    GetCheckAt(i, &class_ids, &target);
+    result.AddCheck(class_ids, target);
+    result.SetCountAt(result.NumberOfChecks() - 1, count);
+  }
+  return result.raw();
+}
+
+
 RawICData* ICData::New(const Function& owner,
                        const String& target_name,
                        const Array& arguments_descriptor,