Pass a second type argument vector to all type instantiation calls in the VM.

Pass a second type argument vector to all type instantiation calls in the VM.
With generic methods, uninstantiated types will require 2 instantiators, one
reflecting the class type arguments (as of today) and one reflecting the
function type arguments (new).
This is work in progress and the second instantiator is always null for now.

R=asiva@google.com

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

[regis, 2017-04-07 00:34:23]

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

[regis, 2017-04-07 00:34:24]

Apologies for the large size, but passing one more argument is touching a lot of
files and I do not see how to effectively chop this cl in smaller chunks. This
is mainly plumbing, though.

[zra, 2017-04-07 17:36:31]

Do we really have to pass an extra argument for the case where we know there
aren't any arguments? Shouldn't we be specializing for the case where there are
no function type arguments since that will probably be the common case?

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/constant_propaga...
File runtime/vm/constant_propagator.cc (right):

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/constant_propaga...
runtime/vm/constant_propagator.cc:920: // IsRawInstantiatedRaw separately for
each genericity.
'IsRawInstantiatedRaw' is this a typo?

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:451: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
Could you use an ldm that doesn't adjust SP?

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:504: __ ldr(kInstantiatorTypeArgumentsReg,
Address(SP, 1 * kWordSize));
ditto if these are adjacent registers.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:584: __ Push(R1);  // Store instantiator
type arguments.
PushList

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:611: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
ldm?

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:616: __ Push(R1);                         
   // Instantiator type arguments.
PushList

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:662: __ Push(R1);  // Store instantiator
type arguments.
PushList

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:681: __ Pop(R2);               // Remove
pushed function type arguments.
PopList

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:692: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
ldm?

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:697: __ Push(R1);                         
 // Instantiator type arguments.
PushList

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:709: __ Pop(R2);  // Remove pushed
function type arguments.
PopList

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:443: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
ldp to load a pair.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:496: __
ldr(kInstantiatorTypeArgumentsReg, Address(SP, 1 * kWordSize));
ditto

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:576: __ Push(R1);  // Store instantiator
type arguments.
PushPair

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:604: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
ldp

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:609: __ Push(R1);                       
     // Instantiator type arguments.
PushPair

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:655: __ Push(R1);  // Store instantiator
type arguments.
PushPair

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:674: __ Pop(R2);               // Remove
pushed function type arguments.
PopPair

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:685: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
ldp

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:690: __ Push(R1);                       
     // Instantiator type arguments.
PushPair

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:702: __ Pop(R2);  // Remove pushed
function type arguments.
PopPair

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_mips.cc:617: __ b(&done);
(optional) Branch delay slot can be used here.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:2649: __
Push(instantiator_type_args_reg);  // Push instantiator type arguments.
PushList

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:2729: __
Push(instantiator_type_args_reg);  // Push instantiator type arguments.
PushList

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm64.cc:2371: __
Push(instantiator_type_args_reg);  // Push instantiator type arguments.
PushPair

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm64.cc:2452: __
Push(instantiator_type_args_reg);  // Push instantiator type arguments.
PushPair

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:5: #include <map>
It looks like this is not used.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:7: #include <string>
ditto

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1615: if (rA == 0 || null_value !=
instantiator_type_args ||
parens around clauses.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1622: if (instantiations->ptr()->data()[i] ==
instantiator_type_args &&
ditto

[regis, 2017-04-07 22:33:42]

On 2017/04/07 17:36:31, zra wrote:
> Do we really have to pass an extra argument for the case where we know there
> aren't any arguments? Shouldn't we be specializing for the case where there
are
> no function type arguments since that will probably be the common case?
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/constant_propaga...
> File runtime/vm/constant_propagator.cc (right):
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/constant_propaga...
> runtime/vm/constant_propagator.cc:920: // IsRawInstantiatedRaw separately for
> each genericity.
> 'IsRawInstantiatedRaw' is this a typo?
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> File runtime/vm/flow_graph_compiler_arm.cc (right):
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:451: __ ldr(R1, Address(SP, 1 *
> kWordSize));  // Get instantiator type args.
> Could you use an ldm that doesn't adjust SP?
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:504: __
ldr(kInstantiatorTypeArgumentsReg,
> Address(SP, 1 * kWordSize));
> ditto if these are adjacent registers.
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:584: __ Push(R1);  // Store instantiator
> type arguments.
> PushList
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:611: __ ldr(R1, Address(SP, 1 *
> kWordSize));  // Get instantiator type args.
> ldm?
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:616: __ Push(R1);                       
 
>    // Instantiator type arguments.
> PushList
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:662: __ Push(R1);  // Store instantiator
> type arguments.
> PushList
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:681: __ Pop(R2);               // Remove
> pushed function type arguments.
> PopList
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:692: __ ldr(R1, Address(SP, 1 *
> kWordSize));  // Get instantiator type args.
> ldm?
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:697: __ Push(R1);                       
 
>  // Instantiator type arguments.
> PushList
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:709: __ Pop(R2);  // Remove pushed
> function type arguments.
> PopList
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> File runtime/vm/flow_graph_compiler_arm64.cc (right):
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:443: __ ldr(R1, Address(SP, 1 *
> kWordSize));  // Get instantiator type args.
> ldp to load a pair.
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:496: __
> ldr(kInstantiatorTypeArgumentsReg, Address(SP, 1 * kWordSize));
> ditto
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:576: __ Push(R1);  // Store
instantiator
> type arguments.
> PushPair
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:604: __ ldr(R1, Address(SP, 1 *
> kWordSize));  // Get instantiator type args.
> ldp
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:609: __ Push(R1);                     
 
>      // Instantiator type arguments.
> PushPair
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:655: __ Push(R1);  // Store
instantiator
> type arguments.
> PushPair
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:674: __ Pop(R2);               //
Remove
> pushed function type arguments.
> PopPair
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:685: __ ldr(R1, Address(SP, 1 *
> kWordSize));  // Get instantiator type args.
> ldp
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:690: __ Push(R1);                     
 
>      // Instantiator type arguments.
> PushPair
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm64.cc:702: __ Pop(R2);  // Remove pushed
> function type arguments.
> PopPair
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> File runtime/vm/flow_graph_compiler_mips.cc (right):
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_mips.cc:617: __ b(&done);
> (optional) Branch delay slot can be used here.
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
> File runtime/vm/intermediate_language_arm.cc (right):
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
> runtime/vm/intermediate_language_arm.cc:2649: __
> Push(instantiator_type_args_reg);  // Push instantiator type arguments.
> PushList
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
> runtime/vm/intermediate_language_arm.cc:2729: __
> Push(instantiator_type_args_reg);  // Push instantiator type arguments.
> PushList
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
> File runtime/vm/intermediate_language_arm64.cc (right):
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
> runtime/vm/intermediate_language_arm64.cc:2371: __
> Push(instantiator_type_args_reg);  // Push instantiator type arguments.
> PushPair
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
> runtime/vm/intermediate_language_arm64.cc:2452: __
> Push(instantiator_type_args_reg);  // Push instantiator type arguments.
> PushPair
> 
> https://codereview.chromium.org/2799373002/diff/1/runtime/vm/kernel_to_il.cc
> File runtime/vm/kernel_to_il.cc (right):
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/kernel_to_il.cc#...
> runtime/vm/kernel_to_il.cc:5: #include <map>
> It looks like this is not used.
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/kernel_to_il.cc#...
> runtime/vm/kernel_to_il.cc:7: #include <string>
> ditto
> 
> https://codereview.chromium.org/2799373002/diff/1/runtime/vm/simulator_dbc.cc
> File runtime/vm/simulator_dbc.cc (right):
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/simulator_dbc.cc...
> runtime/vm/simulator_dbc.cc:1615: if (rA == 0 || null_value !=
> instantiator_type_args ||
> parens around clauses.
> 
>
https://codereview.chromium.org/2799373002/diff/1/runtime/vm/simulator_dbc.cc...
> runtime/vm/simulator_dbc.cc:1622: if (instantiations->ptr()->data()[i] ==
> instantiator_type_args &&
> ditto

Thanks for the review!

Each one of the runtime calls touched by this cl is very expensive. Pushing one
more value in the call is not going to make any difference. So I prefer to keep
it simple and not duplicate code for the case where the function type arguments
are null.


I will address each comment later and will use pairs where possible. However, on
arm, I would have to swap (renumber) the registers to be able to use ldm and
PushList, but I do not want to do it right away. I'll add a TODO.

Thanks,
Regis

[siva, 2017-04-10 22:04:55]

LGTM with some comments.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:776: type.InstantiateFrom(instantiator_type_args,
function_type_args,
why not use Object::null_type_arguments() instead of function_type_args

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

https://codereview.chromium.org/2799373002/diff/1/runtime/lib/object.cc#newco...
runtime/lib/object.cc:231: type, null_type_arguments, null_type_arguments,
&bound_error);
ditto question about using Object::null_type_arguments() instead of the local
null_type_arguments

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_ia32.cc:247: __ popl(temp_reg);      // Discard.
Does it make sense to use
__ Drop(4, temp_reg);
here, it has the smarts to do popl or adjust ESP based on number of pops.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_mips.cc:204: // A2: function type arguments (if
used).
doesn't seem to document that A3 is clobbered, I see such comments in other
functions below.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:222: __ PushObject(Object::null_object());
Do you think it helps to comment as, here and other places where we pass null
for these parameters?

__PushObject(Object::null_object());  // Null instantiator type argument.
__PushObject(Object::null_object());  // Null function type argument.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:243: __ popq(temp_reg);      // Discard.
Does it make sense to use
__ Drop(4, temp_reg);
here, it has the smarts to do a popq or RSP adjustment depending on the number
of slots.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:1604: bool IsRawInstantiatedRaw(intptr_t len) const {
Why does the name repeat Raw twice, why not just
IsRawInstantiated

[regis, 2017-04-11 04:23:09]

Thanks!

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

https://codereview.chromium.org/2799373002/diff/1/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:776: type.InstantiateFrom(instantiator_type_args,
function_type_args,
On 2017/04/10 22:04:55, siva wrote:
> why not use Object::null_type_arguments() instead of function_type_args

Done.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/lib/object.cc#newco...
runtime/lib/object.cc:231: type, null_type_arguments, null_type_arguments,
&bound_error);
On 2017/04/10 22:04:55, siva wrote:
> ditto question about using Object::null_type_arguments() instead of the local
> null_type_arguments

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/constant_propaga...
File runtime/vm/constant_propagator.cc (right):

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/constant_propaga...
runtime/vm/constant_propagator.cc:920: // IsRawInstantiatedRaw separately for
each genericity.
On 2017/04/07 17:36:29, zra wrote:
> 'IsRawInstantiatedRaw' is this a typo?

No :-)
It is used on line 902 if both instantiators are null, but we could use it for
each one separately. I wrote type_arguments().IsRawInstantiatedRaw() to be
clearer.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:451: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
On 2017/04/07 17:36:29, zra wrote:
> Could you use an ldm that doesn't adjust SP?

I'd prefer now, as I would need to renumber the registers for proper ordering.
Added a TODO.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:504: __ ldr(kInstantiatorTypeArgumentsReg,
Address(SP, 1 * kWordSize));
On 2017/04/07 17:36:30, zra wrote:
> ditto if these are adjacent registers.

ditto

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:584: __ Push(R1);  // Store instantiator
type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PushList

Added TODO.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:611: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
On 2017/04/07 17:36:30, zra wrote:
> ldm?

ditto

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:616: __ Push(R1);                         
   // Instantiator type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PushList

ditto

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:662: __ Push(R1);  // Store instantiator
type arguments.
On 2017/04/07 17:36:29, zra wrote:
> PushList

ditto

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:681: __ Pop(R2);               // Remove
pushed function type arguments.
On 2017/04/07 17:36:29, zra wrote:
> PopList

ditto

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:692: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
On 2017/04/07 17:36:29, zra wrote:
> ldm?

ditto

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:697: __ Push(R1);                         
 // Instantiator type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PushList

ditto

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:709: __ Pop(R2);  // Remove pushed
function type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PopList

ditto

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:443: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
On 2017/04/07 17:36:30, zra wrote:
> ldp to load a pair.

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:496: __
ldr(kInstantiatorTypeArgumentsReg, Address(SP, 1 * kWordSize));
On 2017/04/07 17:36:30, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:576: __ Push(R1);  // Store instantiator
type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PushPair

Done.

However, the argument names 'first' and 'second' of PushPair and PopPair are
very confusing. If the idea is to simulate 2 pushes, PushPair pushing 'first'
first and 'second' second, then PopPair should do the same: if PushPair(first,
second) === Push(first); Push(second) then I would expect to call PopPair(first,
second) == Pop(first); Pop(second), but PopPair(first, second) does actually
Pop(second); Pop(first).
So if I want to undo a PushPair(R1, R0), I have to do a PopPair(R1, R0) and not
a PopPair(R0, R1), despite the names of the arguments.
I wanted to rename the arguments to high and low, which are much less confusing
to me. But to keep the same order as for ldp(rt, rt2), I renamed them to low and
high, and fixed the single use of PushPair/PopPair I could find.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:604: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
On 2017/04/07 17:36:30, zra wrote:
> ldp

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:609: __ Push(R1);                       
     // Instantiator type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PushPair

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:655: __ Push(R1);  // Store instantiator
type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PushPair

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:674: __ Pop(R2);               // Remove
pushed function type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PopPair

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:685: __ ldr(R1, Address(SP, 1 *
kWordSize));  // Get instantiator type args.
On 2017/04/07 17:36:30, zra wrote:
> ldp

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:690: __ Push(R1);                       
     // Instantiator type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PushPair

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm64.cc:702: __ Pop(R2);  // Remove pushed
function type arguments.
On 2017/04/07 17:36:30, zra wrote:
> PopPair

Done.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_ia32.cc:247: __ popl(temp_reg);      // Discard.
On 2017/04/10 22:04:55, siva wrote:
> Does it make sense to use
> __ Drop(4, temp_reg);
> here, it has the smarts to do popl or adjust ESP based on number of pops.

Per the comments, we need to restore the receiver. So I replaced the first 2
popl with Drop(2).

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_mips.cc:204: // A2: function type arguments (if
used).
On 2017/04/10 22:04:55, siva wrote:
> doesn't seem to document that A3 is clobbered, I see such comments in other
> functions below.

Added.

I do not think that these comments are all accurate or complete. I corrected a
few in this cl, but I would not trust them.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_mips.cc:617: __ b(&done);
On 2017/04/07 17:36:31, zra wrote:
> (optional) Branch delay slot can be used here.

Done.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:222: __ PushObject(Object::null_object());
On 2017/04/10 22:04:55, siva wrote:
> Do you think it helps to comment as, here and other places where we pass null
> for these parameters?
> 
> __PushObject(Object::null_object());  // Null instantiator type argument.
> __PushObject(Object::null_object());  // Null function type argument.

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:243: __ popq(temp_reg);      // Discard.
On 2017/04/10 22:04:55, siva wrote:
> Does it make sense to use
> __ Drop(4, temp_reg);
> here, it has the smarts to do a popq or RSP adjustment depending on the number
> of slots.

Replaced first 2 popq with Drop(2), since we restore the receiver.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:2649: __
Push(instantiator_type_args_reg);  // Push instantiator type arguments.
On 2017/04/07 17:36:31, zra wrote:
> PushList

Added:
  // TODO(regis): Renumber registers and use PushList.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:2729: __
Push(instantiator_type_args_reg);  // Push instantiator type arguments.
On 2017/04/07 17:36:31, zra wrote:
> PushList

Added:
  // TODO(regis): Renumber registers and use PushList.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm64.cc:2371: __
Push(instantiator_type_args_reg);  // Push instantiator type arguments.
On 2017/04/07 17:36:31, zra wrote:
> PushPair

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm64.cc:2452: __
Push(instantiator_type_args_reg);  // Push instantiator type arguments.
On 2017/04/07 17:36:31, zra wrote:
> PushPair

Done.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:5: #include <map>
On 2017/04/07 17:36:31, zra wrote:
> It looks like this is not used.

Removed

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:7: #include <string>
On 2017/04/07 17:36:31, zra wrote:
> ditto

I can compile with <string> removed, but git cl upload complains. I put it back.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:1604: bool IsRawInstantiatedRaw(intptr_t len) const {
On 2017/04/10 22:04:55, siva wrote:
> Why does the name repeat Raw twice, why not just
> IsRawInstantiated

A raw vector is a vector consisting of Dynamic only. What the name means is "is
this vector becoming raw when it is instantiated from a raw vector?"
I've renamed it to IsRawWhenInstantiatedFromRaw.

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

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1615: if (rA == 0 || null_value !=
instantiator_type_args ||
On 2017/04/07 17:36:31, zra wrote:
> parens around clauses.

Done.

https://codereview.chromium.org/2799373002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1622: if (instantiations->ptr()->data()[i] ==
instantiator_type_args &&
On 2017/04/07 17:36:31, zra wrote:
> ditto

Done.

[regis, 2017-04-11 04:25:39]

Description was changed from

==========
Pass a second type argument vector to all type instantiation calls in the VM.
With generic methods, uninstantiated types will require 2 instantiators, one
reflecting the class type arguments (as of today) and one reflecting the
function type arguments (new).
This is work in progress and the second instantiator is always null for now.
==========

to

==========
Pass a second type argument vector to all type instantiation calls in the VM.
With generic methods, uninstantiated types will require 2 instantiators, one
reflecting the class type arguments (as of today) and one reflecting the
function type arguments (new).
This is work in progress and the second instantiator is always null for now.

R=asiva@google.com

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

[regis, 2017-04-11 04:25:41]

Committed patchset #2 (id:20001) manually as
d0a7bad1210b79a32d96c2d5954430da24bdd741 (presubmit successful).

[erikcorry, 2017-05-10 11:28:40]

erikcorry@google.com changed reviewers:
+ erikcorry@google.com

[erikcorry, 2017-05-10 11:28:41]

Was this expected to regress checked mode performance by 5-10%?

[regis, 2017-05-10 11:39:54]

On 2017/05/10 11:28:41, erikcorry wrote:
> Was this expected to regress checked mode performance by 5-10%?

Nothing comes for free. The performance hit is actually not as bad as I
expected, since mostly artificial benchmarks like megamorphic and deltablue are
impacted in the range you mention.

We can always revisit and see if new optimizations are possible once generic
functions are fully functional, but we are not there yet.