Pass type argument vector to generic functions (if --reify-generic-functions is

Pass type argument vector to generic functions (if --reify-generic-functions is
true, still false per default).
Work in progress, not functional yet (passed vector is still ignored in callee).

R=vegorov@google.com, zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/9aafded095a820faa99df451e8e1ae0ab6f85f29

[regis, 2017-05-02 21:30:39]

regis@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com, vegorov@google.com, zra@google.com

[regis, 2017-05-02 21:30:40]

This is in a shape decent enough to be committed. Let me know what you think.

Thanks,
Regis

[zra, 2017-05-03 15:03:25]

https://codereview.chromium.org/2859673002/diff/1/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/lib/function.cc#new...
runtime/lib/function.cc:33: Closure::CheckedHandle(zone,
arguments->NativeArgAt(0));
The stuff in this file is inconsistent about passing the zone when we allocate
handles.  It looks like there is always a zone passed in to DEFINE_NATIVE_ENTRY
functions.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:230: call->ArgumentCount() - (call->type_args_len()
> 0 ? 1 : 0),
This pattern gets repeated a lot. I'd recommend moving it to a utility function
on ArgumentsDescriptor or the call instructions, and you can include a comment
there saying why you need the adjustment.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:662: const intptr_t kTypeArgsLen = 0;
ASSERT(call->type_args_len() == kTypeArgsLen) ?

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1529: const intptr_t kTypeArgsLen = 0;
ditto.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1622: const intptr_t kTypeArgsLen = 0;
ditto.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1686: const intptr_t kTypeArgsLen = 0;
ditto.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:3174:
Array::Handle(ArgumentsDescriptor::New(kTypeArgsLen, kNumArgs)));
Array::Handle(Z, ...)

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/dart_entry.cc#ne...
runtime/vm/dart_entry.cc:539:
String::Handle(isolate_lib.PrivateName(Symbols::_RawReceivePortImpl()));
Pass the zone here?

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:243: instance_call()->type_args_len(),
instance_call()->ArgumentCount(),
Does this one need to be adjusted?

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:4815: if (IsNull()) return 0;
Please use curly braces.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/stub_code_arm64.cc
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/stub_code_arm64....
runtime/vm/stub_code_arm64.cc:636: __ AddImmediate(R2, R2, Smi::RawValue(1)); 
// Include the type arguments.
Can you use the cinc instruction here instead of a branch?

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/stub_code_arm64....
runtime/vm/stub_code_arm64.cc:1302: __ AddImmediate(R2, R2, Smi::RawValue(1)); 
// Include the type arguments.
ditto

[Vyacheslav Egorov (Google), 2017-05-04 12:12:23]

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1200: intptr_t type_args_len,
I wonder if triple of <type-args-len, pushed_argc, argument_names> should be
wrapped into a structure:

struct ArgumentsInfo {
  public:
    const Array& ToArgumentDescriptor();

  private:
  ...
};

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2759: intptr_t type_args_len_;
I suggest the following refactoring of call instructions:

Introduce a common super class and move everything related to arguments to this
super class:

template<intptr_t kInputCount>
class TemplateDartCall : public TemplateDefinition<kInputCount, Throws> {
 public:
  virtual intptr_t ArgumentCount() const { return arguments_->length(); }
  virtual PushArgumentInstr* PushArgumentAt(intptr_t index) const {
    return (*arguments_)[index];
  }

  intptr_t type_args_len() const { return type_args_len_; }
  const Array& argument_names() const { return argument_names_; }

 private:
   intptr_t type_args_len;
   ZoneGrowableArray<PushArgumentInstr*>* arguments_;
   const Array& argument_names; 
};

Additionally this common super class can have a helper method for repetitive
code patterns, e.g. just like Zach suggested it certainly needs a method like
this:

  const Array& GetArgumentDescriptor() const;

[regis, 2017-05-09 18:31:23]

Thanks for the feedback, ptal.

-- Regis

https://codereview.chromium.org/2859673002/diff/1/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/lib/function.cc#new...
runtime/lib/function.cc:33: Closure::CheckedHandle(zone,
arguments->NativeArgAt(0));
On 2017/05/03 15:03:24, zra wrote:
> The stuff in this file is inconsistent about passing the zone when we allocate
> handles.  It looks like there is always a zone passed in to
DEFINE_NATIVE_ENTRY
> functions.

It looks like 'zone' is indeed passed in and is a stack zone. I added 'zone'
where it was missing.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:230: call->ArgumentCount() - (call->type_args_len()
> 0 ? 1 : 0),
On 2017/05/03 15:03:24, zra wrote:
> This pattern gets repeated a lot. I'd recommend moving it to a utility
function
> on ArgumentsDescriptor or the call instructions, and you can include a comment
> there saying why you need the adjustment.

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:662: const intptr_t kTypeArgsLen = 0;
On 2017/05/03 15:03:24, zra wrote:
> ASSERT(call->type_args_len() == kTypeArgsLen) ?

Done, although arguments names and others were not checked either.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1529: const intptr_t kTypeArgsLen = 0;
On 2017/05/03 15:03:24, zra wrote:
> ditto.

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1622: const intptr_t kTypeArgsLen = 0;
On 2017/05/03 15:03:24, zra wrote:
> ditto.

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:1686: const intptr_t kTypeArgsLen = 0;
On 2017/05/03 15:03:24, zra wrote:
> ditto.

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:3174:
Array::Handle(ArgumentsDescriptor::New(kTypeArgsLen, kNumArgs)));
On 2017/05/03 15:03:24, zra wrote:
> Array::Handle(Z, ...)

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/dart_entry.cc
File runtime/vm/dart_entry.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/dart_entry.cc#ne...
runtime/vm/dart_entry.cc:539:
String::Handle(isolate_lib.PrivateName(Symbols::_RawReceivePortImpl()));
On 2017/05/03 15:03:24, zra wrote:
> Pass the zone here?

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1200: intptr_t type_args_len,
On 2017/05/04 12:12:22, Vyacheslav Egorov (Google) wrote:
> I wonder if triple of <type-args-len, pushed_argc, argument_names> should be
> wrapped into a structure:
> 
> struct ArgumentsInfo {
>   public:
>     const Array& ToArgumentDescriptor();
> 
>   private:
>   ...
> }; 
> 

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:2759: intptr_t type_args_len_;
On 2017/05/04 12:12:23, Vyacheslav Egorov (Google) wrote:
> I suggest the following refactoring of call instructions:
> 
> Introduce a common super class and move everything related to arguments to
this
> super class:
> 
> template<intptr_t kInputCount>
> class TemplateDartCall : public TemplateDefinition<kInputCount, Throws> {
>  public:
>   virtual intptr_t ArgumentCount() const { return arguments_->length(); }
>   virtual PushArgumentInstr* PushArgumentAt(intptr_t index) const {
>     return (*arguments_)[index];
>   }
> 
>   intptr_t type_args_len() const { return type_args_len_; }
>   const Array& argument_names() const { return argument_names_; }
> 
>  private:
>    intptr_t type_args_len;
>    ZoneGrowableArray<PushArgumentInstr*>* arguments_;
>    const Array& argument_names; 
> };
> 
> Additionally this common super class can have a helper method for repetitive
> code patterns, e.g. just like Zach suggested it certainly needs a method like
> this:
> 
>   const Array& GetArgumentDescriptor() const;

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:243: instance_call()->type_args_len(),
instance_call()->ArgumentCount(),
On 2017/05/03 15:03:24, zra wrote:
> Does this one need to be adjusted?

Good catch! I missed this one. One more reason to add helper functions.
I also used a ZoneHandle.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:4815: if (IsNull()) return 0;
On 2017/05/03 15:03:25, zra wrote:
> Please use curly braces.

Done.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/stub_code_arm64.cc
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/stub_code_arm64....
runtime/vm/stub_code_arm64.cc:636: __ AddImmediate(R2, R2, Smi::RawValue(1)); 
// Include the type arguments.
On 2017/05/03 15:03:25, zra wrote:
> Can you use the cinc instruction here instead of a branch?

Done. Used csinc.

https://codereview.chromium.org/2859673002/diff/1/runtime/vm/stub_code_arm64....
runtime/vm/stub_code_arm64.cc:1302: __ AddImmediate(R2, R2, Smi::RawValue(1)); 
// Include the type arguments.
On 2017/05/03 15:03:25, zra wrote:
> ditto

Done.

[zra, 2017-05-11 04:14:21]

https://codereview.chromium.org/2859673002/diff/20001/runtime/lib/invocation_...
File runtime/lib/invocation_mirror_patch.dart (right):

https://codereview.chromium.org/2859673002/diff/20001/runtime/lib/invocation_...
runtime/lib/invocation_mirror_patch.dart:69: int numPositionalArguments =
_argumentsDescriptor[_POSITIONAL_COUNT];
Just checking that it is intentional that the index into _argumentDescriptor is
changing from 1 to 2.

https://codereview.chromium.org/2859673002/diff/20001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/2859673002/diff/20001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:1351: zone, ArgumentsDescriptor::New(0, args.Length(),
arg_names));
Maybe to be consistent with other call-sites:

const int kTypeArgsLen = 0;

https://codereview.chromium.org/2859673002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2859673002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2719: const Array&
ToArgumentsDescriptor(Zone* zone) const {
Why does this not follow the convention of returning a raw pointer?

https://codereview.chromium.org/2859673002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2760: virtual const Array& argument_names()
const { return argument_names_; }
ditto

https://codereview.chromium.org/2859673002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2762: virtual const Array&
GetArgumentsDescriptor(Zone* zone) const {
ditto

[regis, 2017-05-11 09:55:11]

Thanks!

https://codereview.chromium.org/2859673002/diff/20001/runtime/lib/invocation_...
File runtime/lib/invocation_mirror_patch.dart (right):

https://codereview.chromium.org/2859673002/diff/20001/runtime/lib/invocation_...
runtime/lib/invocation_mirror_patch.dart:69: int numPositionalArguments =
_argumentsDescriptor[_POSITIONAL_COUNT];
On 2017/05/11 04:14:20, zra wrote:
> Just checking that it is intentional that the index into _argumentDescriptor
is
> changing from 1 to 2.

Yes, a new entry was inserted at index 0 to represent the type argument vector
length.

https://codereview.chromium.org/2859673002/diff/20001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/2859673002/diff/20001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:1351: zone, ArgumentsDescriptor::New(0, args.Length(),
arg_names));
On 2017/05/11 04:14:20, zra wrote:
> Maybe to be consistent with other call-sites:
> 
> const int kTypeArgsLen = 0;

Done here and below.

https://codereview.chromium.org/2859673002/diff/40001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2859673002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2719: const Array&
ToArgumentsDescriptor(Zone* zone) const {
On 2017/05/11 04:14:21, zra wrote:
> Why does this not follow the convention of returning a raw pointer?

No particular reason :-)
Changed.

https://codereview.chromium.org/2859673002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2760: virtual const Array& argument_names()
const { return argument_names_; }
On 2017/05/11 04:14:21, zra wrote:
> ditto

There is no need to unwrap and rewrap the input here. Note that there are many
locations in the parser and ast where we return handles.

https://codereview.chromium.org/2859673002/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2762: virtual const Array&
GetArgumentsDescriptor(Zone* zone) const {
On 2017/05/11 04:14:21, zra wrote:
> ditto

Done.

[zra, 2017-05-13 06:11:11]

lgtm but since it's a big change, others should finish reviewing as well.

[Vyacheslav Egorov (Google), 2017-05-13 22:20:14]

lgtm

https://codereview.chromium.org/2859673002/diff/80001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:1529: const intptr_t kNumArgs = args.Length();
using k... prefix for a value that is not constant (e.g. it changes over time)
is a bit confusing. 

I think k... prefix should only be used for true constants, e.g. variables that
have the same value always.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:116: const Array& args_desc_array =
This pattern is repeated - maybe call instruction can have a method:

RawFunction* ...CallInstr::ResolveForRecieverClass(const Class& cls)

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:1725: if (instr->type_args_len() > 0) {
Should not be possible. These are operators, no way to pass type arguments
there.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/assembler_db...
File runtime/vm/assembler_dbc_test.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/assembler_db...
runtime/vm/assembler_dbc_test.cc:83: Array::Handle(ArgumentsDescriptor::New(0,
2));
might be worth commenting what each argument means:

/* type_args_len = */ 0, /* arguments count = */ 0

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/code_patcher...
File runtime/vm/code_patcher_arm64_test.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/code_patcher...
runtime/vm/code_patcher_arm64_test.cc:37:
Array::Handle(ArgumentsDescriptor::New(0, 1, Object::null_array()));
ditto re commenting the meaning of parameters.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/dart_entry.h
File runtime/vm/dart_entry.h (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/dart_entry.h...
runtime/vm/dart_entry.h:37: intptr_t TypeArgsLen() const;
question: would it make sense to call this TypeArgsCount() just so that we use
word "count" for everything consistently?

though the current code is not very consistent: we use "number" and "count"
interchangeably

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2522: (node->arguments()->type_args_len() > 0 ?
2 : 1) +
maybe node->arguments()->LengthWithTypeArgs() + 1 instead?

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2562: node->arguments()->LengthWithTypeArgs());
I think it should be +1 here because we also push closure itself as a receiver?

(original code seems to have a "bug" here which did not cause any problems
because array is growable)

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:866: // TODO(regis): Consider type arguments in
arguments.
maybe also add 

if (function.IsGeneric()) return false;

for now to avoid hitting assertions / breaking inliner?

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2792: virtual intptr_t FirstParamIndex()
const {
Any reason for this to be virtual? It is not overriden now.

I think only ArgumentCount, PushArgumentAt and token_pos need to be virtual.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2795: virtual intptr_t
ArgumentCountWithoutTypeArgs() const {
ditto

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2803: virtual intptr_t type_args_len() const
{ return type_args_len_; }
ditto

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2804: virtual const Array& argument_names()
const { return argument_names_; }
ditto

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2806: virtual RawArray*
GetArgumentsDescriptor() const {
ditto

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/runtime_entr...
File runtime/vm/runtime_entry.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/runtime_entr...
runtime/vm/runtime_entry.cc:1148: ASSERT(!old_target.IsGeneric());
I think to guarantee this assertion we need to have a check in UnlinkedCall

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/runtime_entr...
runtime/vm/runtime_entry.cc:1248: if (!target_function.IsNull() &&
!target_function.HasOptionalParameters()) {
I think this needs to check for !target_function.IsGeneric()

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_ia...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:526: __ cmpl(EAX, Immediate(0));
__ cmpl(FieldAddress(), Immediate(0))

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:1173: __ cmpl(EAX, Immediate(0));
ditto

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_x6...
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:571: __ cmpq(RAX, Immediate(0));
__ cmpq(FieldAddress(...), Immediate(0))

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:1229: __ cmpq(RAX, Immediate(0));
ditto

[regis, 2017-05-18 21:02:13]

Thank you very much!

https://codereview.chromium.org/2859673002/diff/80001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:1529: const intptr_t kNumArgs = args.Length();
On 2017/05/13 22:20:13, Vyacheslav Egorov (Google) wrote:
> using k... prefix for a value that is not constant (e.g. it changes over time)
> is a bit confusing. 
> 
> I think k... prefix should only be used for true constants, e.g. variables
that
> have the same value always.

Agreed. Fixed to num_args. I'm not sure why I had changed this...

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:116: const Array& args_desc_array =
On 2017/05/13 22:20:13, Vyacheslav Egorov (Google) wrote:
> This pattern is repeated - maybe call instruction can have a method:
> 
> RawFunction* ...CallInstr::ResolveForRecieverClass(const Class& cls)
> 

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:1725: if (instr->type_args_len() > 0) {
On 2017/05/13 22:20:13, Vyacheslav Egorov (Google) wrote:
> Should not be possible. These are operators, no way to pass type arguments
> there.

What I thought. Replaced by assert.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/assembler_db...
File runtime/vm/assembler_dbc_test.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/assembler_db...
runtime/vm/assembler_dbc_test.cc:83: Array::Handle(ArgumentsDescriptor::New(0,
2));
On 2017/05/13 22:20:13, Vyacheslav Egorov (Google) wrote:
> might be worth commenting what each argument means:
> 
> /* type_args_len = */ 0, /* arguments count = */ 0

Declared named constants.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/code_patcher...
File runtime/vm/code_patcher_arm64_test.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/code_patcher...
runtime/vm/code_patcher_arm64_test.cc:37:
Array::Handle(ArgumentsDescriptor::New(0, 1, Object::null_array()));
On 2017/05/13 22:20:13, Vyacheslav Egorov (Google) wrote:
> ditto re commenting the meaning of parameters.

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/dart_entry.h
File runtime/vm/dart_entry.h (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/dart_entry.h...
runtime/vm/dart_entry.h:37: intptr_t TypeArgsLen() const;
On 2017/05/13 22:20:13, Vyacheslav Egorov (Google) wrote:
> question: would it make sense to call this TypeArgsCount() just so that we use
> word "count" for everything consistently?
> 
> though the current code is not very consistent: we use "number" and "count"
> interchangeably

We could use 'count' instead of 'num' in most places (maybe in a later cleanup
cl?), however, I choose 'len' because a non-zero len adds only 1 to the
arguments 'count'. I felt that TypeArgsCount() would be misleading.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2522: (node->arguments()->type_args_len() > 0 ?
2 : 1) +
On 2017/05/13 22:20:13, Vyacheslav Egorov (Google) wrote:
> maybe node->arguments()->LengthWithTypeArgs() + 1 instead?

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2562: node->arguments()->LengthWithTypeArgs());
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> I think it should be +1 here because we also push closure itself as a
receiver?
> 
> (original code seems to have a "bug" here which did not cause any problems
> because array is growable)

Good catch.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:866: // TODO(regis): Consider type arguments in
arguments.
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> maybe also add 
> 
> if (function.IsGeneric()) return false;
> 
> for now to avoid hitting assertions / breaking inliner?

I suspect that many generic functions of the sdk get inlined. They are currently
called without type arguments. I prefer to return only if type arguments are
passed. I already started to work on the inliner in another cl.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2792: virtual intptr_t FirstParamIndex()
const {
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> Any reason for this to be virtual? It is not overriden now.
> 
> I think only ArgumentCount, PushArgumentAt and token_pos need to be virtual.

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2795: virtual intptr_t
ArgumentCountWithoutTypeArgs() const {
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2803: virtual intptr_t type_args_len() const
{ return type_args_len_; }
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2804: virtual const Array& argument_names()
const { return argument_names_; }
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2806: virtual RawArray*
GetArgumentsDescriptor() const {
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/runtime_entr...
File runtime/vm/runtime_entry.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/runtime_entr...
runtime/vm/runtime_entry.cc:1148: ASSERT(!old_target.IsGeneric());
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> I think to guarantee this assertion we need to have a check in UnlinkedCall

Oops. Done. Thanks.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/runtime_entr...
runtime/vm/runtime_entry.cc:1248: if (!target_function.IsNull() &&
!target_function.HasOptionalParameters()) {
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> I think this needs to check for !target_function.IsGeneric()

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_ia...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:526: __ cmpl(EAX, Immediate(0));
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> __ cmpl(FieldAddress(), Immediate(0))

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:1173: __ cmpl(EAX, Immediate(0));
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_x6...
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:571: __ cmpq(RAX, Immediate(0));
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> __ cmpq(FieldAddress(...), Immediate(0))

Done.

https://codereview.chromium.org/2859673002/diff/80001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:1229: __ cmpq(RAX, Immediate(0));
On 2017/05/13 22:20:14, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

[regis, 2017-05-18 21:03:53]

Description was changed from

==========
Pass type argument vector to generic functions (if --reify-generic-functions is
true, still false per default).
Work in progress, not functional yet (passed vector is still ignored in callee).
==========

to

==========
Pass type argument vector to generic functions (if --reify-generic-functions is
true, still false per default).
Work in progress, not functional yet (passed vector is still ignored in callee).

R=vegorov@google.com, zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/9aafded095a820faa99df451e8e1ae0ab6f85f29
==========

[regis, 2017-05-18 21:03:54]

Committed patchset #6 (id:100001) manually as
9aafded095a820faa99df451e8e1ae0ab6f85f29 (presubmit successful).