Use off-heap data for type feedback in PolymorphicInstanceCallInstr

runtime/vm/intermediate_language.cc

Index: runtime/vm/intermediate_language.cc
diff --git a/runtime/vm/intermediate_language.cc b/runtime/vm/intermediate_language.cc
index 1ed759b493a3e8fd8574373539a3366afc7cdde3..27c7461e76c8280d9df85dc470ecffbcc17a4dd2 100644
--- a/runtime/vm/intermediate_language.cc
+++ b/runtime/vm/intermediate_language.cc
@@ -3167,10 +3167,66 @@ bool InstanceCallInstr::MatchesCoreName(const String& name) {
 
 bool PolymorphicInstanceCallInstr::HasSingleRecognizedTarget() const {
   if (FLAG_precompiled_mode && with_checks()) return false;
+  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;
+}
 
-  return ic_data().HasOneTarget() &&
-         (MethodRecognizer::RecognizeKind(Function::Handle(
-              ic_data().GetTargetAt(0))) != MethodRecognizer::kUnknown);
+
+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;
 }
 
 
@@ -3178,10 +3234,9 @@ bool PolymorphicInstanceCallInstr::HasSingleRecognizedTarget() const {
 // 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(),
@@ -3190,7 +3245,7 @@ void PolymorphicInstanceCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
   }
 
   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());
 }
@@ -3198,22 +3253,29 @@ void PolymorphicInstanceCallInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
 
 
 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();
@@ -3226,12 +3288,20 @@ RawType* PolymorphicInstanceCallInstr::ComputeRuntimeType(
 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.

+
+  // 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 ICData& new_ic_data =
-      FlowGraphCompiler::TrySpecializeICDataByReceiverCid(*ic_data(),
-                                                          receiver_cid);
-  if (new_ic_data.raw() == ic_data()->raw()) {
+  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;
   }
@@ -3239,21 +3309,21 @@ Definition* InstanceCallInstr::Canonicalize(FlowGraph* flow_graph) {
   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);
     }