Check for a passed-in type argument vector in the prolog of generic functions.

Check for a passed-in type argument vector in the prolog of generic functions.
Do this in unoptimized code only, when --reify-generic-functions is specified.
This is still work in progress, and support in optimizer, in inliner, in DBC,
in kernel to ir, and other areas, will follow.
Many small fixes and added todos.

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

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

[regis, 2017-06-13 23:23:37]

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

[rmacnak, 2017-06-14 00:14:20]

Some superficial comments

https://codereview.chromium.org/2941643002/diff/1/runtime/lib/function_patch....
File runtime/lib/function_patch.dart (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/lib/function_patch....
runtime/lib/function_patch.dart:11: static apply(Function function, List
positionalArguments,
Really this should be updated to something like

apply(function, positionArguments, [namedArguments, typeArguments])

but I'd avoid doing so until it is specifically asked for.

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/native_entry.h
File runtime/vm/native_entry.h (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/native_entry.h#n...
runtime/vm/native_entry.h:56: ASSERT(retval->IsDartInstance() ||
retval->IsTypeArguments());               \
This makes me nervous. Is it possible to use a runtime function instead of a
native function?

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

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:14705: return zone->PrintToString("[this stub]");  // Not
yet recorded.
ASSERT stub code not initialized? I think there's already a way to check that's
used by the profiler.

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

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:1919: // TODO(regis): Pass type argument vector. Not clear
when this code is used.
Something like this:

func<X, Y, X>(w) => null;

class A {
  var field = func;
}

main() {
  new A().field<int, int, int>(2);
}

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

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:836: // TODO(regis): Do we need to support type
arguments when calling from C++?
Or mirrors? *cringe*

Let's not add this until there's a specific request for it.

[regis, 2017-06-14 21:02:05]

Thanks for the initial comments.
I'll try to use a runtime call instead of a native call and fix other issues as
noted. Stay tuned.

https://codereview.chromium.org/2941643002/diff/1/runtime/lib/function_patch....
File runtime/lib/function_patch.dart (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/lib/function_patch....
runtime/lib/function_patch.dart:11: static apply(Function function, List
positionalArguments,
On 2017/06/14 00:14:20, rmacnak wrote:
> Really this should be updated to something like
> 
> apply(function, positionArguments, [namedArguments, typeArguments])
> 
> but I'd avoid doing so until it is specifically asked for.

Agreed. It is something that needs to be defined in the API, similarly to the
invocation mirror in noSuchMethod.
The TODO is merely a reminder that things are not complete.

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/native_entry.h
File runtime/vm/native_entry.h (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/native_entry.h#n...
runtime/vm/native_entry.h:56: ASSERT(retval->IsDartInstance() ||
retval->IsTypeArguments());               \
On 2017/06/14 00:14:20, rmacnak wrote:
> This makes me nervous. Is it possible to use a runtime function instead of a
> native function?

I first looked at a runtime call, but a native one seemed easier to implement. I
can have a second look.

As a side note, we pass TypeArguments as arguments to factory methods already
and we do not have asserts checking that all arguments are DartInstance.

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

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:14705: return zone->PrintToString("[this stub]");  // Not
yet recorded.
On 2017/06/14 00:14:20, rmacnak wrote:
> ASSERT stub code not initialized? I think there's already a way to check
that's
> used by the profiler.

I found this in the profiler:
StubCode::HasBeenInitialized()
I suppose that is what you mean. I'll try.

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

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:1919: // TODO(regis): Pass type argument vector. Not clear
when this code is used.
On 2017/06/14 00:14:20, rmacnak wrote:
> Something like this:
> 
> func<X, Y, X>(w) => null;
> 
> class A {
>   var field = func;
> }
> 
> main() {
>   new A().field<int, int, int>(2);
> }

Thanks! This example causes an assert fault in kernel_to_il.cc. So I still have
work to do.

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

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:836: // TODO(regis): Do we need to support type
arguments when calling from C++?
On 2017/06/14 00:14:20, rmacnak wrote:
> Or mirrors? *cringe*
> 
> Let's not add this until there's a specific request for it.

Agreed.

[rmacnak, 2017-06-15 00:03:27]

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/native_entry.h
File runtime/vm/native_entry.h (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/native_entry.h#n...
runtime/vm/native_entry.h:56: ASSERT(retval->IsDartInstance() ||
retval->IsTypeArguments());               \
On 2017/06/14 21:02:04, regis wrote:
> On 2017/06/14 00:14:20, rmacnak wrote:
> > This makes me nervous. Is it possible to use a runtime function instead of a
> > native function?
> 
> I first looked at a runtime call, but a native one seemed easier to implement.
I
> can have a second look.
> 
> As a side note, we pass TypeArguments as arguments to factory methods already
> and we do not have asserts checking that all arguments are DartInstance.

Ah, I hadn't thought of factory type arguments. I withdraw my concern here.

[Vyacheslav Egorov (Google), 2017-06-15 14:05:44]

https://codereview.chromium.org/2941643002/diff/1/runtime/lib/object_patch.dart
File runtime/lib/object_patch.dart (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/lib/object_patch.da...
runtime/lib/object_patch.dart:63: static
_prependTypeArguments(functionTypeArguments, parentTypeArguments, len)
Is it possible to put this into dart:_internal instead of placing it on Object?
I find it a bit confusing that we put a lot of helpers on Object.

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:3902: if (is_top_level_sequence &&
FLAG_reify_generic_functions &&
maybe reoder condition to be more logical, moving FLAG_ to the very start e.g. 

FLAG_reify_generic_functions && is_top_level_sequence && function.IsGeneric()

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

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:1919: // TODO(regis): Pass type argument vector. Not clear
when this code is used.
On 2017/06/14 21:02:05, regis wrote:
> On 2017/06/14 00:14:20, rmacnak wrote:
> > Something like this:
> > 
> > func<X, Y, X>(w) => null;
> > 
> > class A {
> >   var field = func;
> > }
> > 
> > main() {
> >   new A().field<int, int, int>(2);
> > }
> 
> Thanks! This example causes an assert fault in kernel_to_il.cc. So I still
have
> work to do.

Is this even legal? I thought the consensus was that you can't tear-off without
passing type arguments:

illegal: var field = func;
legal: var field = func<int, int, int>

Maybe we change semantics again.

[regis, 2017-06-15 21:38:27]

Thanks for the comments so far. No updated version yet to look at. Stay tuned.

https://codereview.chromium.org/2941643002/diff/1/runtime/lib/object_patch.dart
File runtime/lib/object_patch.dart (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/lib/object_patch.da...
runtime/lib/object_patch.dart:63: static
_prependTypeArguments(functionTypeArguments, parentTypeArguments, len)
On 2017/06/15 14:05:44, Vyacheslav Egorov (Google) wrote:
> Is it possible to put this into dart:_internal instead of placing it on
Object?
> I find it a bit confusing that we put a lot of helpers on Object.

Yes, definitely. I did not know of the existence of internal_patch.dart.

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:3902: if (is_top_level_sequence &&
FLAG_reify_generic_functions &&
On 2017/06/15 14:05:44, Vyacheslav Egorov (Google) wrote:
> maybe reoder condition to be more logical, moving FLAG_ to the very start e.g.

> 
> FLAG_reify_generic_functions && is_top_level_sequence && function.IsGeneric()

Will do.

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

https://codereview.chromium.org/2941643002/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:1919: // TODO(regis): Pass type argument vector. Not clear
when this code is used.
On 2017/06/15 14:05:44, Vyacheslav Egorov (Google) wrote:
> On 2017/06/14 21:02:05, regis wrote:
> > On 2017/06/14 00:14:20, rmacnak wrote:
> > > Something like this:
> > > 
> > > func<X, Y, X>(w) => null;
> > > 
> > > class A {
> > >   var field = func;
> > > }
> > > 
> > > main() {
> > >   new A().field<int, int, int>(2);
> > > }
> > 
> > Thanks! This example causes an assert fault in kernel_to_il.cc. So I still
> have
> > work to do.
> 
> Is this even legal? I thought the consensus was that you can't tear-off
without
> passing type arguments:
> 
> illegal: var field = func;
> legal: var field = func<int, int, int>
> 
> Maybe we change semantics again. 

Yes, the first version required you to pass the type arguments at tear-off time,
but, luckily, this has changed. You pass them now at closure calling time, which
is much more flexible and actually easier to implement.

[regis, 2017-06-15 23:41:56]

PTAL

I think I addressed all your comments.

Thanks,
Regis

[regis, 2017-06-16 00:43:11]

On 2017/06/15 23:41:56, regis wrote:
> PTAL
> 
> I think I addressed all your comments.
> 
> Thanks,
> Regis

Mmm, I realize now that internal_patch.dart may not be the correct source for
dart:_internal, since _prependTypeArguments is still found as a static on
Object. Can you point me to the right place?

Thanks,
Regis

[Vyacheslav Egorov (Google), 2017-06-16 11:16:54]

Hi Regis,

I think internal_patch.dart should be the place to put it. I am not sure why we
would be able to find it on Object once you moved it away into
internal_patch.dart. Maybe something was not rebuilt or our lookup is broken.

[regis, 2017-06-16 17:59:14]

On 2017/06/16 11:16:54, Vyacheslav Egorov (Google) wrote:
> Hi Regis,
> 
> I think internal_patch.dart should be the place to put it. I am not sure why
we
> would be able to find it on Object once you moved it away into
> internal_patch.dart. Maybe something was not rebuilt or our lookup is broken.

Slava,

You are right. I was using the wrong test and _prependTypeArguments was not
called. This is now fixed in patch set 3. The many differences are due to the
fact that I synced.

Thanks,
Regis

[regis, 2017-06-19 20:12:24]

Friendly ping.

[Vyacheslav Egorov (Google), 2017-06-20 15:21:41]

lgtm

maybe wait for one other reviewer - as this code is non trivial.

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:120: ((function.kernel_offset() > 0) ||
This should probably be:

  return (function.kernel_offset() > 0) ||
    (!FLAG_reify_generic_functions && 
     ((function.kind() == RawFunction::kNoSuchMethodDispatcher) ||
      (function.kind() == RawFunction::kInvokeFieldDispatcher)));

because functions with kernel_offset() > 0 should always go into the Kernel
front-end no matter whether reify_generic_functions is on or off.

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

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/dart_entry.c...
runtime/vm/dart_entry.cc:441: ArgumentsDescriptor::NewNonCached(0, i, false);
maybe:

/*type_args_len=*/ 0

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

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:13110: " "
maybe:

" type-args-len: " ...

to remove this stray " "

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/object_test....
runtime/vm/object_test.cc:2573: first_parameter_index, num_parameters,
type_args_slot, first_frame_index,
Does it make sense to make a test *with* type arguments, to check if we compute
things correctly?

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/scopes.cc#ne...
runtime/vm/scopes.cc:238: if ((pos == num_parameters) && (type_args_slot == 1))
{
can this instead be:

if (variable->name().raw() == Symbols::FunctionTypeArgumentsVar().raw()) {
  ASSERT(pos == num_parameters);
}

? then you don't even need to pass type_args_slot down.

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/scopes.h#new...
runtime/vm/scopes.h:317: int type_args_slot,
why is this not a boolean like:

bool needs_type_args_slot

[rmacnak, 2017-06-20 17:20:46]

lgtm

https://codereview.chromium.org/2941643002/diff/40001/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/lib/object.cc#n...
runtime/lib/object.cc:375: TypeArguments::Handle(zone, TypeArguments::New(len,
Heap::kOld));
Maybe kNew? With repeated calls of the of same generic closure, only the first
type will become the canonical type and the rest will just grow old space.
Similar to what we did for Object.runtimeType.

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:35: // TODO(regis): Do we need to explicitly filter
generic functions here?
Only if there are errors we don't detect at link time.

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

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:836: // TODO(regis): Do we need to support type
arguments when calling from C++?
We'd get a nicer error message if this is checked on the C++ side instead.

[regis, 2017-06-21 04:39:04]

Thank you both!
I'll commit tomorrow.

https://codereview.chromium.org/2941643002/diff/40001/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/lib/object.cc#n...
runtime/lib/object.cc:375: TypeArguments::Handle(zone, TypeArguments::New(len,
Heap::kOld));
On 2017/06/20 17:20:46, rmacnak wrote:
> Maybe kNew? With repeated calls of the of same generic closure, only the first
> type will become the canonical type and the rest will just grow old space.
> Similar to what we did for Object.runtimeType.

Done.

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:120: ((function.kernel_offset() > 0) ||
On 2017/06/20 15:21:41, Vyacheslav Egorov (Google) wrote:
> This should probably be:
> 
>   return (function.kernel_offset() > 0) ||
>     (!FLAG_reify_generic_functions && 
>      ((function.kind() == RawFunction::kNoSuchMethodDispatcher) ||
>       (function.kind() == RawFunction::kInvokeFieldDispatcher)));
> 
> because functions with kernel_offset() > 0 should always go into the Kernel
> front-end no matter whether reify_generic_functions is on or off.

Done.

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

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/dart_entry.c...
runtime/vm/dart_entry.cc:441: ArgumentsDescriptor::NewNonCached(0, i, false);
On 2017/06/20 15:21:41, Vyacheslav Egorov (Google) wrote:
> maybe:
> 
> /*type_args_len=*/ 0 

Done.

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:35: // TODO(regis): Do we need to explicitly filter
generic functions here?
On 2017/06/20 17:20:46, rmacnak wrote:
> Only if there are errors we don't detect at link time.

I updated, but kept the TODO here for now.

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

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:13110: " "
On 2017/06/20 15:21:41, Vyacheslav Egorov (Google) wrote:
> maybe:
> 
> " type-args-len: " ...
> 
> to remove this stray " "

Done.

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/object_test....
runtime/vm/object_test.cc:2573: first_parameter_index, num_parameters,
type_args_slot, first_frame_index,
On 2017/06/20 15:21:41, Vyacheslav Egorov (Google) wrote:
> Does it make sense to make a test *with* type arguments, to check if we
compute
> things correctly?

I updated the test with a type args variable and found a bug. Thanks!

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/scopes.cc#ne...
runtime/vm/scopes.cc:238: if ((pos == num_parameters) && (type_args_slot == 1))
{
On 2017/06/20 15:21:41, Vyacheslav Egorov (Google) wrote:
> can this instead be:
> 
> if (variable->name().raw() == Symbols::FunctionTypeArgumentsVar().raw()) {
>   ASSERT(pos == num_parameters);
> }
> 
> ? then you don't even need to pass type_args_slot down.

Good point. Thanks. I never revisited this code after it worked...

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/scopes.h#new...
runtime/vm/scopes.h:317: int type_args_slot,
On 2017/06/20 15:21:41, Vyacheslav Egorov (Google) wrote:
> why is this not a boolean like:
> 
> bool needs_type_args_slot

Removed, per your suggestion in scopes.cc.

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

https://codereview.chromium.org/2941643002/diff/40001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:836: // TODO(regis): Do we need to support type
arguments when calling from C++?
On 2017/06/20 17:20:46, rmacnak wrote:
> We'd get a nicer error message if this is checked on the C++ side instead.

Agreed. Since we are not yet supporting calling from C++ with type arguments. I
left a comment here and added an ApiError in DartEntry::InvokeFunction in case
type args are passed.

[regis, 2017-06-21 16:02:21]

Description was changed from

==========
Check for a passed-in type argument vector in the prolog of generic functions.
Do this in unoptimized code only, when --reify-generic-functions is specified.
This is still work in progress, and support in optimizer, in inliner, in DBC,
in kernel to ir, and other areas, will follow.
Many small fixes and added todos.
==========

to

==========
Check for a passed-in type argument vector in the prolog of generic functions.
Do this in unoptimized code only, when --reify-generic-functions is specified.
This is still work in progress, and support in optimizer, in inliner, in DBC,
in kernel to ir, and other areas, will follow.
Many small fixes and added todos.

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

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

[regis, 2017-06-21 16:02:22]

Committed patchset #4 (id:60001) manually as
a62107cfaf8442a54ca7a87b096244cff992965f (presubmit successful).