Use off-heap data for type feedback in PolymorphicInstanceCallInstr

runtime/vm/flow_graph_inliner.cc

Index: runtime/vm/flow_graph_inliner.cc
diff --git a/runtime/vm/flow_graph_inliner.cc b/runtime/vm/flow_graph_inliner.cc
index 9601ea3b8c3791541febc72fa2b3cc4e69cde06c..afaf2bb4c4ba435c3680e3d6aca46f61f964cd41 100644
--- a/runtime/vm/flow_graph_inliner.cc
+++ b/runtime/vm/flow_graph_inliner.cc
@@ -290,9 +290,7 @@ class CallSites : public ValueObject {
     GrowableArray<intptr_t> instance_call_counts(num_instance_calls);
     for (intptr_t i = 0; i < num_instance_calls; ++i) {
       const intptr_t aggregate_count =
-          instance_calls_[i + instance_call_start_ix]
-              .call->ic_data()
-              .AggregateCount();
+          instance_calls_[i + instance_call_start_ix].call->CallCount();
       instance_call_counts.Add(aggregate_count);
       if (aggregate_count > max_count) max_count = aggregate_count;
     }
@@ -343,11 +341,11 @@ class CallSites : public ValueObject {
         if (current->IsPolymorphicInstanceCall()) {
           PolymorphicInstanceCallInstr* instance_call =
               current->AsPolymorphicInstanceCall();
-          target = instance_call->ic_data().GetTargetAt(0);
+          target ^= instance_call->FirstTarget().raw();

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

Why are we recording only a single target here? Makes no sense.

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

Agreed, but that's how I found it.

           call = instance_call;
         } else if (current->IsStaticCall()) {
           StaticCallInstr* static_call = current->AsStaticCall();
-          target = static_call->function().raw();
+          target ^= static_call->function().raw();
           call = static_call;
         } else if (current->IsClosureCall()) {
           // TODO(srdjan): Add data for closure calls.
@@ -389,16 +387,14 @@ class CallSites : public ValueObject {
               current->AsPolymorphicInstanceCall();
           if (!inline_only_recognized_methods ||
               instance_call->HasSingleRecognizedTarget() ||
-              instance_call->ic_data()
-                  .HasOnlyDispatcherOrImplicitAccessorTargets()) {
+              instance_call->HasOnlyDispatcherOrImplicitAccessorTargets()) {
             instance_calls_.Add(InstanceCallInfo(instance_call, graph));
           } else {
             // Method not inlined because inlining too deep and method
             // not recognized.
             if (FLAG_print_inlining_tree) {
               const Function* caller = &graph->function();
-              const Function* target = &Function::ZoneHandle(
-                  instance_call->ic_data().GetTargetAt(0));
+              const Function* target = &instance_call->FirstTarget();

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

Ditto.

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

Acknowledged.

               inlined_info->Add(InlinedInfo(caller, target, depth + 1,
                                             instance_call, "Too deep"));
             }
@@ -491,10 +487,10 @@ class PolymorphicInliner : public ValueObject {
   CallSiteInliner* const owner_;
   PolymorphicInstanceCallInstr* const call_;
   const intptr_t num_variants_;
-  GrowableArray<CidRangeTarget> variants_;
+  const PolymorphicTargets& variants_;
 
-  GrowableArray<CidRangeTarget> inlined_variants_;
-  GrowableArray<CidRangeTarget> non_inlined_variants_;
+  PolymorphicTargets* inlined_variants_;

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

Why not embed this by value? Even if object is zone allocated you can still use
it as a value object.

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

I didn't know you could do that.  Done for inlined_variants_.

In this case of non_inlined_variants_ we want to attach it to an instruction, so
the lifetime doesn't work and it has to be in the zone.

+  PolymorphicTargets* non_inlined_variants_;
   GrowableArray<BlockEntryInstr*> inlined_entries_;
   InlineExitCollector* exit_collector_;
 
@@ -1305,8 +1301,7 @@ class CallSiteInliner : public ValueObject {
         continue;
       }
 
-      const ICData& ic_data = call->ic_data();
-      const Function& target = Function::ZoneHandle(ic_data.GetTargetAt(0));
+      const Function& target = call->FirstTarget();

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

Why are we looking only on the FirstTarget()? Is this the case when
PolymorphicInstanceCall has single target?

You see - the naming is confusing.

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

Made a synonym called MostPopularTarget.  HTH.

       if (!inliner_->AlwaysInline(target) &&
           (call_info[call_idx].ratio * 100) < FLAG_inlining_hotness) {
         if (trace_inlining()) {
@@ -1447,10 +1442,10 @@ PolymorphicInliner::PolymorphicInliner(CallSiteInliner* owner,
                                        intptr_t caller_inlining_id)
     : owner_(owner),
       call_(call),
-      num_variants_(call->ic_data().NumberOfChecks()),
-      variants_(num_variants_),
-      inlined_variants_(num_variants_),
-      non_inlined_variants_(num_variants_),
+      num_variants_(call->NumberOfChecks()),
+      variants_(call->targets_),
+      inlined_variants_(new (zone()) PolymorphicTargets(zone())),
+      non_inlined_variants_(new (zone()) PolymorphicTargets(zone())),
       inlined_entries_(num_variants_),
       exit_collector_(new (Z) InlineExitCollector(owner->caller_graph(), call)),
       caller_function_(caller_function),
@@ -1482,8 +1477,8 @@ intptr_t PolymorphicInliner::AllocateBlockId() const {
 //
 //   * JoinEntry: the inlined body is shared and this is a subsequent variant.
 bool PolymorphicInliner::CheckInlinedDuplicate(const Function& target) {
-  for (intptr_t i = 0; i < inlined_variants_.length(); ++i) {
-    if ((target.raw() == inlined_variants_[i].target->raw()) &&
+  for (intptr_t i = 0; i < inlined_variants_->length(); ++i) {
+    if ((target.raw() == inlined_variants_->At(i).target->raw()) &&
         !MethodRecognizer::PolymorphicTarget(target)) {
       // The call target is shared with a previous inlined variant.  Share
       // the graph.  This requires a join block at the entry, and edge-split
@@ -1534,8 +1529,8 @@ bool PolymorphicInliner::CheckInlinedDuplicate(const Function& target) {
 
 
 bool PolymorphicInliner::CheckNonInlinedDuplicate(const Function& target) {
-  for (intptr_t i = 0; i < non_inlined_variants_.length(); ++i) {
-    if (target.raw() == non_inlined_variants_[i].target->raw()) {
+  for (intptr_t i = 0; i < non_inlined_variants_->length(); ++i) {
+    if (target.raw() == non_inlined_variants_->At(i).target->raw()) {
       return true;
     }
   }
@@ -1559,8 +1554,8 @@ bool PolymorphicInliner::TryInliningPoly(const CidRangeTarget& range) {
   }
   InlinedCallData call_data(call_, &arguments, caller_function_,
                             caller_inlining_id_);
-  if (!owner_->TryInlining(*range.target,
-                           call_->instance_call()->argument_names(),
+  Function& target = Function::ZoneHandle(zone(), range.target->raw());
+  if (!owner_->TryInlining(target, call_->instance_call()->argument_names(),
                            &call_data)) {
     return false;
   }
@@ -1698,17 +1693,17 @@ TargetEntryInstr* PolymorphicInliner::BuildDecisionGraph() {
   load_cid->set_ssa_temp_index(owner_->caller_graph()->alloc_ssa_temp_index());
   cursor = AppendInstruction(cursor, load_cid);
   bool follow_with_deopt = false;
-  for (intptr_t i = 0; i < inlined_variants_.length(); ++i) {
-    const CidRangeTarget& variant = inlined_variants_[i];
+  for (intptr_t i = 0; i < inlined_variants_->length(); ++i) {
+    const CidRangeTarget& variant = inlined_variants_->At(i);
     bool test_is_range = (variant.cid_start != variant.cid_end);
-    bool is_last_test = (i == inlined_variants_.length() - 1);
+    bool is_last_test = (i == inlined_variants_->length() - 1);
     // 1. Guard the body with a class id check.  We don't need any check if
     // it's the last test and global analysis has told us that the call is
     // complete.  TODO(erikcorry): Enhance CheckClassIdInstr so it can take an
     // arbitrary CidRangeTarget.  Currently we don't go into this branch if the
     // last test is a range test - instead we set the follow_with_deopt flag.
     if (is_last_test && (!test_is_range || call_->complete()) &&
-        non_inlined_variants_.is_empty()) {
+        non_inlined_variants_->is_empty()) {
       // If it is the last variant use a check class id instruction which can
       // deoptimize, followed unconditionally by the body. Omit the check if
       // we know that we have covered all possible classes.
@@ -1884,7 +1879,7 @@ TargetEntryInstr* PolymorphicInliner::BuildDecisionGraph() {
   }
 
   // Handle any non-inlined variants.
-  if (!non_inlined_variants_.is_empty()) {
+  if (!non_inlined_variants_->is_empty()) {
     // Move push arguments of the call.
     for (intptr_t i = 0; i < call_->ArgumentCount(); ++i) {
       PushArgumentInstr* push = call_->PushArgumentAt(i);
@@ -1893,29 +1888,10 @@ TargetEntryInstr* PolymorphicInliner::BuildDecisionGraph() {
       cursor->LinkTo(push);
       cursor = push;
     }
-    const ICData& old_checks = call_->ic_data();
-    const ICData& new_checks = ICData::ZoneHandle(ICData::New(
-        Function::Handle(old_checks.Owner()),
-        String::Handle(old_checks.target_name()),
-        Array::Handle(old_checks.arguments_descriptor()), old_checks.deopt_id(),
-        1,        // Number of args tested.
-        false));  // is_static_call
-    for (intptr_t i = 0; i < non_inlined_variants_.length(); ++i) {
-      // We are adding all the cids in each range. They will be joined
-      // together again by the PolymorphicInstanceCall instruction, which is a
-      // bit messy.
-      intptr_t count = non_inlined_variants_[i].count;
-
-      for (intptr_t j = non_inlined_variants_[i].cid_start;
-           j <= non_inlined_variants_[i].cid_end; j++) {
-        new_checks.AddReceiverCheck(j, *non_inlined_variants_[i].target, count);
-        count = 0;
-      }
-    }
     PolymorphicInstanceCallInstr* fallback_call =
-        new PolymorphicInstanceCallInstr(call_->instance_call(), new_checks,
-                                         /* with_checks = */ true,
-                                         call_->complete());
+        new PolymorphicInstanceCallInstr(
+            call_->instance_call(), *non_inlined_variants_,
+            /* with_checks = */ true, call_->complete());
     fallback_call->set_ssa_temp_index(
         owner_->caller_graph()->alloc_ssa_temp_index());
     fallback_call->InheritDeoptTarget(zone(), call_);
@@ -1965,13 +1941,12 @@ bool PolymorphicInliner::trace_inlining() const {
 
 
 void PolymorphicInliner::Inline() {
-  // Consider the polymorphic variants in order by frequency.
-  FlowGraphCompiler::SortICDataByCount(call_->ic_data(), &variants_,
-                                       /* drop_smi = */ false);
+  ASSERT(&variants_ == &call_->targets_);
+
   intptr_t total = call_->total_call_count();
   for (intptr_t var_idx = 0; var_idx < variants_.length(); ++var_idx) {
     if (variants_.length() > FLAG_max_polymorphic_checks) {
-      non_inlined_variants_.Add(variants_[var_idx]);
+      non_inlined_variants_->Add(variants_[var_idx]);
       continue;
     }
 
@@ -1979,7 +1954,7 @@ void PolymorphicInliner::Inline() {
     // the last two, because it's a big win if we inline all of them (compiler
     // can see all side effects).
     const bool try_harder = (var_idx >= variants_.length() - 2) &&
-                            non_inlined_variants_.length() == 0;
+                            non_inlined_variants_->length() == 0;
     const Function& target = *variants_[var_idx].target;
     const intptr_t count = variants_[var_idx].count;
 
@@ -1992,7 +1967,7 @@ void PolymorphicInliner::Inline() {
     if (!try_harder && count < (total >> 5)) {
       TRACE_INLINING(
           TracePolyInlining(variants_[var_idx], total, "way too infrequent"));
-      non_inlined_variants_.Add(variants_[var_idx]);
+      non_inlined_variants_->Add(variants_[var_idx]);
       continue;
     }
 
@@ -2000,7 +1975,7 @@ void PolymorphicInliner::Inline() {
     if (CheckInlinedDuplicate(target)) {
       TRACE_INLINING(TracePolyInlining(variants_[var_idx], total,
                                        "duplicate already inlined"));
-      inlined_variants_.Add(variants_[var_idx]);
+      inlined_variants_->Add(variants_[var_idx]);
       continue;
     }
 
@@ -2010,7 +1985,7 @@ void PolymorphicInliner::Inline() {
     if (!try_harder && count < (total >> (small ? 4 : 3))) {
       TRACE_INLINING(
           TracePolyInlining(variants_[var_idx], total, "too infrequent"));
-      non_inlined_variants_.Add(variants_[var_idx]);
+      non_inlined_variants_->Add(variants_[var_idx]);
       continue;
     }
 
@@ -2020,23 +1995,23 @@ void PolymorphicInliner::Inline() {
     if (CheckNonInlinedDuplicate(target)) {
       TRACE_INLINING(
           TracePolyInlining(variants_[var_idx], total, "already not inlined"));
-      non_inlined_variants_.Add(variants_[var_idx]);
+      non_inlined_variants_->Add(variants_[var_idx]);
       continue;
     }
 
     // Make an inlining decision.
     if (TryInliningPoly(variants_[var_idx])) {
       TRACE_INLINING(TracePolyInlining(variants_[var_idx], total, "inlined"));
-      inlined_variants_.Add(variants_[var_idx]);
+      inlined_variants_->Add(variants_[var_idx]);
     } else {
       TRACE_INLINING(
           TracePolyInlining(variants_[var_idx], total, "not inlined"));
-      non_inlined_variants_.Add(variants_[var_idx]);
+      non_inlined_variants_->Add(variants_[var_idx]);
     }
   }
 
   // If there are no inlined variants, leave the call in place.
-  if (inlined_variants_.is_empty()) return;
+  if (inlined_variants_->is_empty()) return;
 
   // Now build a decision tree (a DAG because of shared inline variants) and
   // inline it at the call site.