[vm] Decouple flow graph building of dynamic invocation forwarders from kernel reading

Change-Id: I65f8fb5f3817630870e752ecb14ff3fb0506e7ef
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/96711
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Samir Jindel <sjindel@google.com>
diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
index 13de1df..a3bb265 100644
--- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
+++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc
@@ -387,162 +387,6 @@
   parsed_function()->SetDefaultFunctionTypeArguments(default_types);
 }
 
-Fragment StreamingFlowGraphBuilder::PushAllArguments(PushedArguments* pushed) {
-  FunctionNodeHelper function_node_helper(this);
-  function_node_helper.SetNext(FunctionNodeHelper::kTypeParameters);
-
-  Fragment body;
-
-  const intptr_t num_type_params = ReadListLength();
-  if (num_type_params > 0) {
-    // Skip type arguments.
-    for (intptr_t i = 0; i < num_type_params; ++i) {
-      TypeParameterHelper helper(this);
-      helper.Finish();
-    }
-
-    body += LoadLocal(parsed_function()->function_type_arguments());
-    body += PushArgument();
-    pushed->type_args_len = num_type_params;
-  }
-  function_node_helper.SetJustRead(FunctionNodeHelper::kTypeParameters);
-  function_node_helper.ReadUntilExcluding(
-      FunctionNodeHelper::kPositionalParameters);
-
-  // Push receiver.
-  body += LoadLocal(scopes()->this_variable);
-  body += PushArgument();
-
-  // Push positional parameters.
-  const intptr_t num_positional_params = ReadListLength();
-  for (intptr_t i = 0; i < num_positional_params; ++i) {
-    // ith variable offset.
-    const intptr_t offset = ReaderOffset();
-    SkipVariableDeclaration();
-
-    LocalVariable* param = LookupVariable(offset + data_program_offset_);
-    body += LoadLocal(param);
-    body += PushArgument();
-  }
-
-  // Push named parameters.
-  const intptr_t num_named_params = ReadListLength();
-  pushed->argument_names = Array::New(num_named_params, Heap::kOld);
-  for (intptr_t i = 0; i < num_named_params; ++i) {
-    // ith variable offset.
-    const intptr_t offset = ReaderOffset();
-    SkipVariableDeclaration();
-
-    LocalVariable* param = LookupVariable(offset + data_program_offset_);
-    pushed->argument_names.SetAt(i, param->name());
-    body += LoadLocal(param);
-    body += PushArgument();
-  }
-
-  pushed->argument_count = num_positional_params + num_named_params + 1;
-
-  return body;
-}
-
-FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfDynamicInvocationForwarder() {
-  const Function& dart_function = parsed_function()->function();
-
-  // The prologue builder needs the default parameter values.
-  SetupDefaultParameterValues();
-  // BuildDefaultTypeHandling needs default function type arguments.
-  ReadDefaultFunctionTypeArguments(dart_function);
-
-  B->graph_entry_ = new (Z) GraphEntryInstr(*parsed_function(), B->osr_id_);
-
-  auto normal_entry = B->BuildFunctionEntry(B->graph_entry_);
-  B->graph_entry_->set_normal_entry(normal_entry);
-
-  PrologueInfo prologue_info(-1, -1);
-  auto instruction_cursor = B->BuildPrologue(normal_entry, &prologue_info);
-
-  Fragment body;
-  if (!dart_function.is_native()) {
-    body += B->CheckStackOverflowInPrologue(dart_function.token_pos());
-  }
-
-  ASSERT(parsed_function()->node_sequence()->scope()->num_context_variables() ==
-         0);
-
-  FunctionNodeHelper function_node_helper(this);
-  function_node_helper.ReadUntilExcluding(FunctionNodeHelper::kTypeParameters);
-  const intptr_t type_parameters_offset = ReaderOffset();
-  function_node_helper.ReadUntilExcluding(
-      FunctionNodeHelper::kPositionalParameters);
-  intptr_t first_parameter_offset = -1;
-  {
-    AlternativeReadingScope alt(&reader_);
-    intptr_t list_length = ReadListLength();  // read number of positionals.
-    if (list_length > 0) {
-      first_parameter_offset = ReaderOffset() + data_program_offset_;
-    }
-  }
-  USE(first_parameter_offset);
-  // Current position: About to read list of positionals.
-
-  // Should never build a dynamic invocation forwarder for equality
-  // operator.
-  ASSERT(dart_function.name() != Symbols::EqualOperator().raw());
-
-  // Even if the caller did not pass argument vector we would still
-  // call the target with instantiate-to-bounds type arguments.
-  body += B->BuildDefaultTypeHandling(dart_function);
-
-  String& name = String::Handle(Z, dart_function.name());
-  name = Function::DemangleDynamicInvocationForwarderName(name);
-  const Class& owner = Class::Handle(Z, dart_function.Owner());
-  const Function& target =
-      Function::ZoneHandle(Z, owner.LookupDynamicFunction(name));
-  ASSERT(!target.IsNull());
-
-  // Build argument type checks that complement those that are emitted in the
-  // target.
-  {
-    AlternativeReadingScope alt(&reader_);
-    SetOffset(type_parameters_offset);
-    B->BuildArgumentTypeChecks(
-        TypeChecksToBuild::kCheckNonCovariantTypeParameterBounds, &body, &body,
-        nullptr);
-  }
-
-  // Push all arguments and invoke the original method.
-  PushedArguments pushed = {0, 0, Array::ZoneHandle(Z)};
-  {
-    AlternativeReadingScope alt(&reader_);
-    SetOffset(type_parameters_offset);
-    body += PushAllArguments(&pushed);
-  }
-  body += StaticCall(TokenPosition::kNoSource, target, pushed.argument_count,
-                     pushed.argument_names, ICData::kNoRebind, nullptr,
-                     pushed.type_args_len);
-
-  // Some IL optimization passes assume that result of operator []= invocation
-  // is never used, so we drop it and replace with an explicit null constant.
-  if (name.raw() == Symbols::AssignIndexToken().raw()) {
-    body += Drop();
-    body += NullConstant();
-  }
-
-  body += Return(TokenPosition::kNoSource);
-
-  instruction_cursor->LinkTo(body.entry);
-
-  GraphEntryInstr* graph_entry = B->graph_entry_;
-  // When compiling for OSR, use a depth first search to find the OSR
-  // entry and make graph entry jump to it instead of normal entry.
-  // Catch entries are always considered reachable, even if they
-  // become unreachable after OSR.
-  if (B->IsCompiledForOsr()) {
-    graph_entry->RelinkToOsrEntry(Z, B->last_used_block_id_ + 1);
-  }
-  return new (Z) FlowGraph(*parsed_function(), graph_entry,
-                           B->last_used_block_id_, prologue_info);
-}
-
 Fragment StreamingFlowGraphBuilder::DebugStepCheckInPrologue(
     const Function& dart_function,
     TokenPosition position) {
@@ -1160,12 +1004,12 @@
       return B->BuildGraphOfFieldAccessor(function);
     }
     case RawFunction::kDynamicInvocationForwarder:
-      if (PeekTag() == kField) {
-        return B->BuildGraphOfFieldAccessor(function);
-      } else {
+      if (PeekTag() != kField) {
         ReadUntilFunctionNode();
-        return BuildGraphOfDynamicInvocationForwarder();
+        SetupDefaultParameterValues();
+        ReadDefaultFunctionTypeArguments(function);
       }
+      return B->BuildGraphOfDynamicInvocationForwarder(function);
     case RawFunction::kMethodExtractor:
       return flow_graph_builder_->BuildGraphOfMethodExtractor(function);
     case RawFunction::kNoSuchMethodDispatcher:
diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h
index c8192c4..bedc54e 100644
--- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h
+++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h
@@ -63,7 +63,6 @@
   Fragment BuildFieldInitializer(NameIndex canonical_name);
   Fragment BuildInitializers(const Class& parent_class);
   FlowGraph* BuildGraphOfFunction(bool constructor);
-  FlowGraph* BuildGraphOfDynamicInvocationForwarder();
 
   Fragment BuildExpression(TokenPosition* position = NULL);
   Fragment BuildStatement();
@@ -196,13 +195,6 @@
       bool use_unchecked_entry = false,
       const CallSiteAttributesMetadata* call_site_attrs = nullptr);
 
-  struct PushedArguments {
-    intptr_t type_args_len;
-    intptr_t argument_count;
-    Array& argument_names;
-  };
-  Fragment PushAllArguments(PushedArguments* pushed);
-
   Fragment ThrowException(TokenPosition position);
   Fragment BooleanNegate();
   Fragment TranslateInstantiatedTypeArguments(
diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc
index 01bd29a..9afb03d 100644
--- a/runtime/vm/compiler/frontend/kernel_to_il.cc
+++ b/runtime/vm/compiler/frontend/kernel_to_il.cc
@@ -1243,6 +1243,35 @@
   return instruction_cursor;
 }
 
+RawArray* FlowGraphBuilder::GetOptionalParameterNames(
+    const Function& function) {
+  if (!function.HasOptionalNamedParameters()) {
+    return Array::null();
+  }
+
+  const intptr_t num_fixed_params = function.num_fixed_parameters();
+  const intptr_t num_opt_params = function.NumOptionalNamedParameters();
+  const auto& names = Array::Handle(Z, Array::New(num_opt_params, Heap::kOld));
+  auto& name = String::Handle(Z);
+  for (intptr_t i = 0; i < num_opt_params; ++i) {
+    name = function.ParameterNameAt(num_fixed_params + i);
+    names.SetAt(i, name);
+  }
+  return names.raw();
+}
+
+Fragment FlowGraphBuilder::PushExplicitParameters(const Function& function) {
+  Fragment instructions;
+  auto scope = parsed_function_->node_sequence()->scope();
+  for (intptr_t i = function.NumImplicitParameters(),
+                n = function.NumParameters();
+       i < n; ++i) {
+    instructions += LoadLocal(scope->VariableAt(i));
+    instructions += PushArgument();
+  }
+  return instructions;
+}
+
 FlowGraph* FlowGraphBuilder::BuildGraphOfMethodExtractor(
     const Function& method) {
   // A method extractor is the implicit getter for a method.
@@ -2001,50 +2030,29 @@
     body += PushArgument();
   }
 
-  // Load all the arguments.
-  auto scope = parsed_function_->node_sequence()->scope();
-
+  // Push receiver.
   if (!target.is_static()) {
     // The context has a fixed shape: a single variable which is the
     // closed-over receiver.
-    body += LoadLocal(scope->VariableAt(0));
+    body +=
+        LoadLocal(parsed_function_->node_sequence()->scope()->VariableAt(0));
     body += LoadNativeField(Slot::Closure_context());
     body += LoadNativeField(
         Slot::GetContextVariableSlotFor(thread_, *scopes_->this_variable));
     body += PushArgument();
   }
 
-  // Positional.
-  intptr_t arg_index = function.NumImplicitParameters();
-  const intptr_t named_argument_count = function.NumOptionalNamedParameters();
-  const intptr_t positional_argument_count = function.NumParameters() -
-                                             function.NumImplicitParameters() -
-                                             named_argument_count;
-  for (intptr_t i = 0; i < positional_argument_count; ++i, ++arg_index) {
-    body += LoadLocal(scope->VariableAt(arg_index));  // ith variable offset.
-    body += PushArgument();
-  }
+  body += PushExplicitParameters(function);
 
-  // Named.
-  Array& argument_names = Array::ZoneHandle(Z);
-  if (named_argument_count > 0) {
-    argument_names = Array::New(named_argument_count, Heap::kOld);
-    String& name = String::Handle(Z);
-    for (intptr_t i = 0; i < named_argument_count; ++i, ++arg_index) {
-      body += LoadLocal(scope->VariableAt(arg_index));
-      body += PushArgument();
-
-      name = function.ParameterNameAt(arg_index);
-      argument_names.SetAt(i, name);
-    }
-  }
-
-  // Forward them to the parent.
-  intptr_t argument_count = positional_argument_count + named_argument_count;
-  if (!parent.is_static()) {
-    ++argument_count;
-  }
+  // Forward parameters to the target.
+  intptr_t argument_count = function.NumParameters() -
+                            function.NumImplicitParameters() +
+                            (target.is_static() ? 0 : 1);
   ASSERT(argument_count == target.NumParameters());
+
+  Array& argument_names =
+      Array::ZoneHandle(Z, GetOptionalParameterNames(function));
+
   body += StaticCall(TokenPosition::kNoSource, target, argument_count,
                      argument_names, ICData::kNoRebind,
                      /* result_type = */ NULL, type_args_len);
@@ -2165,6 +2173,98 @@
                            prologue_info);
 }
 
+FlowGraph* FlowGraphBuilder::BuildGraphOfDynamicInvocationForwarder(
+    const Function& function) {
+  auto& name = String::Handle(Z, function.name());
+  name = Function::DemangleDynamicInvocationForwarderName(name);
+  const auto& owner = Class::Handle(Z, function.Owner());
+  const auto& target =
+      Function::ZoneHandle(Z, owner.LookupDynamicFunction(name));
+  ASSERT(!target.IsNull());
+  ASSERT(!target.IsImplicitGetterFunction());
+
+  if (target.IsImplicitSetterFunction()) {
+    return BuildGraphOfFieldAccessor(function);
+  }
+
+  graph_entry_ = new (Z) GraphEntryInstr(*parsed_function_, osr_id_);
+
+  auto normal_entry = BuildFunctionEntry(graph_entry_);
+  graph_entry_->set_normal_entry(normal_entry);
+
+  PrologueInfo prologue_info(-1, -1);
+  auto instruction_cursor = BuildPrologue(normal_entry, &prologue_info);
+
+  Fragment body;
+  if (!function.is_native()) {
+    body += CheckStackOverflowInPrologue(function.token_pos());
+  }
+
+  ASSERT(parsed_function_->node_sequence()->scope()->num_context_variables() ==
+         0);
+
+  // Should never build a dynamic invocation forwarder for equality
+  // operator.
+  ASSERT(function.name() != Symbols::EqualOperator().raw());
+
+  // Even if the caller did not pass argument vector we would still
+  // call the target with instantiate-to-bounds type arguments.
+  body += BuildDefaultTypeHandling(function);
+
+  // Build argument type checks that complement those that are emitted in the
+  // target.
+  BuildArgumentTypeChecks(
+      TypeChecksToBuild::kCheckNonCovariantTypeParameterBounds, &body, &body,
+      nullptr);
+
+  // Push all arguments and invoke the original method.
+
+  intptr_t type_args_len = 0;
+  if (function.IsGeneric()) {
+    type_args_len = function.NumTypeParameters();
+    ASSERT(parsed_function_->function_type_arguments() != nullptr);
+    body += LoadLocal(parsed_function_->function_type_arguments());
+    body += PushArgument();
+  }
+
+  // Push receiver.
+  ASSERT(function.NumImplicitParameters() == 1);
+  body += LoadLocal(scopes_->this_variable);
+  body += PushArgument();
+
+  body += PushExplicitParameters(function);
+
+  const intptr_t argument_count = function.NumParameters();
+  const auto& argument_names =
+      Array::ZoneHandle(Z, GetOptionalParameterNames(function));
+
+  body += StaticCall(TokenPosition::kNoSource, target, argument_count,
+                     argument_names, ICData::kNoRebind, nullptr, type_args_len);
+
+  // Later optimization passes assume that result of a x.[]=(...) call is not
+  // used. We must guarantee this invariant because violation will lead to an
+  // illegal IL once we replace x.[]=(...) with a sequence that does not
+  // actually produce any value. See http://dartbug.com/29135 for more details.
+  if (name.raw() == Symbols::AssignIndexToken().raw()) {
+    body += Drop();
+    body += NullConstant();
+  }
+
+  body += Return(TokenPosition::kNoSource);
+
+  instruction_cursor->LinkTo(body.entry);
+
+  // When compiling for OSR, use a depth first search to find the OSR
+  // entry and make graph entry jump to it instead of normal entry.
+  // Catch entries are always considered reachable, even if they
+  // become unreachable after OSR.
+  if (IsCompiledForOsr()) {
+    graph_entry_->RelinkToOsrEntry(Z, last_used_block_id_ + 1);
+  }
+  return new (Z) FlowGraph(*parsed_function_, graph_entry_, last_used_block_id_,
+                           prologue_info);
+}
+
 void FlowGraphBuilder::SetCurrentTryCatchBlock(TryCatchBlock* try_catch_block) {
   try_catch_block_ = try_catch_block;
   SetCurrentTryIndex(try_catch_block == nullptr ? kInvalidTryIndex
diff --git a/runtime/vm/compiler/frontend/kernel_to_il.h b/runtime/vm/compiler/frontend/kernel_to_il.h
index fdbae6a..7442db1 100644
--- a/runtime/vm/compiler/frontend/kernel_to_il.h
+++ b/runtime/vm/compiler/frontend/kernel_to_il.h
@@ -91,6 +91,12 @@
   BlockEntryInstr* BuildPrologue(BlockEntryInstr* normal_entry,
                                  PrologueInfo* prologue_info);
 
+  // Return names of optional named parameters of [function].
+  RawArray* GetOptionalParameterNames(const Function& function);
+
+  // Generate fragment which pushes all explicit parameters of [function].
+  Fragment PushExplicitParameters(const Function& function);
+
   FlowGraph* BuildGraphOfMethodExtractor(const Function& method);
   FlowGraph* BuildGraphOfNoSuchMethodDispatcher(const Function& function);
   FlowGraph* BuildGraphOfInvokeFieldDispatcher(const Function& function);
@@ -258,6 +264,8 @@
   // Builds flow graph for implicit closure function (tear-off).
   //
   // ParsedFunction should have the following information:
+  //  - DefaultFunctionTypeArguments()
+  //  - function_type_arguments()
   //  - default_parameter_values()
   //  - is_forwarding_stub()
   //  - forwarding_stub_super_target()
@@ -279,6 +287,21 @@
   //
   FlowGraph* BuildGraphOfFieldAccessor(const Function& function);
 
+  // Builds flow graph of dynamic invocation forwarder.
+  //
+  // ParsedFunction should have the following information:
+  //  - DefaultFunctionTypeArguments()
+  //  - function_type_arguments()
+  //  - default_parameter_values()
+  //  - is_forwarding_stub()
+  //  - forwarding_stub_super_target()
+  //
+  // Scope should be populated with parameter variables including
+  //  - needs_type_check()
+  //  - is_explicit_covariant_parameter()
+  //
+  FlowGraph* BuildGraphOfDynamicInvocationForwarder(const Function& function);
+
   TranslationHelper translation_helper_;
   Thread* thread_;
   Zone* zone_;