Use off-heap data for type feedback in PolymorphicInstanceCallInstr

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/bit_vector.h"
 #include "vm/bootstrap.h"
 #include "vm/compiler.h"
 #include "vm/constant_propagator.h"
@@ skipping 3149 matching lines @@
 }
 
 
 bool InstanceCallInstr::MatchesCoreName(const String& name) {
   return function_name().raw() == Library::PrivateCoreLibName(name).raw();
 }
 
 
 bool PolymorphicInstanceCallInstr::HasSingleRecognizedTarget() const {
   if (FLAG_precompiled_mode && with_checks()) return false;
-
-  return ic_data().HasOneTarget() &&
-         (MethodRecognizer::RecognizeKind(Function::Handle(
-              ic_data().GetTargetAt(0))) != MethodRecognizer::kUnknown);
+  return targets_.HasSingleRecognizedTarget();
 }
 
 
+bool PolymorphicTargets::HasSingleRecognizedTarget() const {

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

Please don't intersperse implementations of two different classes - this makes
it very hard to read this code.

[erikcorry, 2017/04/19 15:06:40]

Done.

+  if (!HasSingleTarget()) return false;
+  return MethodRecognizer::RecognizeKind(FirstTarget()) !=

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

I think FirstTarget() should better be named SingleTarget() to match
HasSingleTarget() name.

[erikcorry, 2017/04/19 15:06:40]

I'd like to leave it because it is sometimes used where there is more than one
target where we for some reason want the first. The reasons are not very good,
but I don't want to expand the scope of this change any more.

+         MethodRecognizer::kUnknown;
+}
+
+
+bool PolymorphicTargets::HasSingleTarget() const {
+  ASSERT(length() != 0);
+  for (int i = 0; i < length(); i++) {
+    if (cid_ranges_[i].target->raw() != cid_ranges_[0].target->raw())
+      return false;
+  }
+  return true;
+}
+
+
+bool PolymorphicInstanceCallInstr::HasOnlyDispatcherOrImplicitAccessorTargets()
+    const {
+  const intptr_t len = targets_.length();
+  Function& target = Function::Handle();
+  for (intptr_t i = 0; i < len; i++) {
+    target ^= targets_[i].target->raw();
+    if (!target.IsDispatcherOrImplicitAccessor()) {
+      return false;
+    }
+  }
+  return true;
+}
+
+
+bool PolymorphicInstanceCallInstr::HasSingleRecognizedCid() const {

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

Why is this function called HasSingleRecognizedCid? (e.g. why is it
"recognized"?).

I think a better name is just IsMonomorphic() or HasSingleReceiverCid() or
something along this lines.

Also the question is why is this not a method on targets themselves (ditto
below).

[erikcorry, 2017/04/19 15:06:40]

Done.

+  if (targets_.length() != 1) return false;
+  return targets_[0].cid_start == targets_[0].cid_end;
+}
+
+
+intptr_t PolymorphicInstanceCallInstr::SingleCid() const {

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

Better name would be MonomorphicReceiverCid().

[erikcorry, 2017/04/19 15:06:40]

Done.

+  ASSERT(HasSingleRecognizedCid());
+  return targets_[0].cid_start;
+}
+
+
+Function& PolymorphicTargets::FirstTarget() const {

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

This should probably be SingleTarget()

[erikcorry, 2017/04/19 15:06:40]

See above.

+  ASSERT(length() != 0);
+  ASSERT(cid_ranges_[0].target->IsZoneHandle());
+  return *cid_ranges_[0].target;
+}
+
+
+intptr_t PolymorphicInstanceCallInstr::CallCount() const {

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

This should probably be just calling the method on the targets called something
like AggregateCallCount().

[erikcorry, 2017/04/19 15:06:40]

Done.

+  intptr_t sum = 0;
+  for (int i = 0; i < targets_.length(); i++) {
+    sum += targets_[i].count;
+  }
+  return sum;
+}
+
+
 // DBC does not support optimizing compiler and thus doesn't emit
 // PolymorphicInstanceCallInstr.
 #if !defined(TARGET_ARCH_DBC)
 void PolymorphicInstanceCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
-  ASSERT(ic_data().NumArgsTested() == 1);
   if (!with_checks()) {
-    ASSERT(ic_data().HasOneTarget());
-    const Function& target = Function::ZoneHandle(ic_data().GetTargetAt(0));
+    ASSERT(HasSingleTarget());
+    const Function& target = Function::ZoneHandle(targets_[0].target->raw());

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

Instead of the targets[0].target access use targets.SingleTarget() here.

[erikcorry, 2017/04/19 15:06:40]

Done.

     compiler->GenerateStaticCall(deopt_id(), instance_call()->token_pos(),
                                  target, instance_call()->ArgumentCount(),
                                  instance_call()->argument_names(), locs(),
                                  ICData::Handle());
     return;
   }
 
   compiler->EmitPolymorphicInstanceCall(
-      ic_data(), instance_call()->ArgumentCount(),
+      targets_, *instance_call(), instance_call()->ArgumentCount(),
       instance_call()->argument_names(), deopt_id(),
       instance_call()->token_pos(), locs(), complete(), total_call_count());
 }
 #endif
 
 
 RawType* PolymorphicInstanceCallInstr::ComputeRuntimeType(
-    const ICData& ic_data) {
+    const PolymorphicTargets& targets) {
   bool is_string = true;
   bool is_integer = true;
   bool is_double = true;
 
-  const intptr_t num_checks = ic_data.NumberOfChecks();
+  const intptr_t num_checks = targets.length();
   for (intptr_t i = 0; i < num_checks; i++) {
-    const intptr_t cid = ic_data.GetReceiverClassIdAt(i);
-    is_string = is_string && RawObject::IsStringClassId(cid);
-    is_integer = is_integer && RawObject::IsIntegerClassId(cid);
-    is_double = is_double && (cid == kDoubleCid);
+    ASSERT(targets[i].target->raw() == targets[0].target->raw());
+    const intptr_t start = targets[i].cid_start;
+    const intptr_t end = targets[i].cid_end;
+    for (intptr_t cid = start; cid <= end; cid++) {
+      is_string = is_string && RawObject::IsStringClassId(cid);
+      is_integer = is_integer && RawObject::IsIntegerClassId(cid);
+      is_double = is_double && (cid == kDoubleCid);
+    }
   }
 
   if (is_string) {
+    ASSERT(!is_integer);
+    ASSERT(!is_double);
     return Type::StringType();
   } else if (is_integer) {
+    ASSERT(!is_double);
     return Type::IntType();
   } else if (is_double) {
     return Type::Double();
   }
 
   return Type::null();
 }
 
 
 Definition* InstanceCallInstr::Canonicalize(FlowGraph* flow_graph) {
   const intptr_t receiver_cid = PushArgumentAt(0)->value()->Type()->ToCid();
 
-  if (!HasICData()) return this;
+  if (ic_data()->NumArgsTested() != 1) return this;

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

Please elaborate why this check is here so that the next person reading this
code knows.

[erikcorry, 2017/04/19 15:06:40]

I can't remember :-/.  I have the benchmarking service running a version with
and without this line, so lets see if it helps.

 
-  const ICData& new_ic_data =
-      FlowGraphCompiler::TrySpecializeICDataByReceiverCid(*ic_data(),
-                                                          receiver_cid);
-  if (new_ic_data.raw() == ic_data()->raw()) {
+  // TODO(erikcorry): Even for cold call sites we could still try to look up
+  // methods when we know the receiver cid. We don't currently do this because
+  // it turns the InstanceCall into a PolymorphicInstanceCall which doesn't get
+  // recognized or inlined when it is cold.

[Vyacheslav Egorov (Google), 2017/04/10 10:59:28]

I don't think coldness has anything to do with it. It's just that we replace it
later in the pipeline - so inlining/specialization does not run again.

[erikcorry, 2017/04/19 15:06:40]

Things seem to work with this check here, that checks for coldness.  The
regression was on NavierStokesTyped.

+  if (ic_data()->NumberOfUsedChecks() == 0) return this;
+
+  const PolymorphicTargets* new_target =
+      FlowGraphCompiler::TrySpecializeByReceiverCid(
+          Array::Handle(flow_graph->zone(), ic_data()->arguments_descriptor()),
+          String::Handle(flow_graph->zone(), ic_data()->target_name()),
+          receiver_cid);
+  if (new_target == NULL) {
     // No specialization.
     return this;
   }
 
   const bool with_checks = false;
   const bool complete = false;
   PolymorphicInstanceCallInstr* specialized = new PolymorphicInstanceCallInstr(
-      this, new_ic_data, with_checks, complete);
+      this, *new_target, with_checks, complete);
   flow_graph->InsertBefore(this, specialized, env(), FlowGraph::kValue);
   return specialized;
 }
 
 
 Definition* PolymorphicInstanceCallInstr::Canonicalize(FlowGraph* flow_graph) {
-  if (!HasSingleRecognizedTarget() || with_checks()) {
+  if (!HasSingleRecognizedTarget()) {
     return this;
   }
 
-  const Function& target = Function::Handle(ic_data().GetTargetAt(0));
+  const Function& target = FirstTarget();
   if (target.recognized_kind() == MethodRecognizer::kObjectRuntimeType) {
     const AbstractType& type =
-        AbstractType::Handle(ComputeRuntimeType(ic_data()));
+        AbstractType::Handle(ComputeRuntimeType(targets_));
     if (!type.IsNull()) {
       return flow_graph->GetConstant(type);
     }
   }
 
   return this;
 }
 
 
 Definition* StaticCallInstr::Canonicalize(FlowGraph* flow_graph) {
@@ skipping 731 matching lines @@
                      "native function '%s' (%" Pd " arguments) cannot be found",
                      native_name().ToCString(), function().NumParameters());
   }
   set_is_auto_scope(auto_setup_scope);
   set_native_c_function(native_function);
 }
 
 #undef __
 
 }  // namespace dart