Store arguments descriptor in ICData. Remove loading of arguments descriptor at unoptimized call si…

Store arguments descriptor in ICData. Remove loading of arguments descriptor at unoptimized call site (the ones using ICData).

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

Committed: https://code.google.com/p/dart/source/detail?r=24239

[zra, 2013-06-19 21:39:03]

arm, mips lgtm

https://codereview.chromium.org/17421003/diff/24009/runtime/vm/stub_code_mips.cc
File runtime/vm/stub_code_mips.cc (right):

https://codereview.chromium.org/17421003/diff/24009/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:1500: __ addiu(SP, SP, Immediate(-5 * kWordSize));
Only need 4 slots now.

https://codereview.chromium.org/17421003/diff/24009/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:1508: __ addiu(SP, SP, Immediate(5 * kWordSize)); 
// Discard argument;
4

https://codereview.chromium.org/17421003/diff/24009/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:1857: __ addiu(SP, SP, Immediate(-2 * kWordSize));
Only need 1 slot now. If there's only one thing we're putting on the stack, then
Push and Pop are fine.

[srdjan, 2013-06-19 22:03:49]

https://codereview.chromium.org/17421003/diff/24009/runtime/vm/stub_code_mips.cc
File runtime/vm/stub_code_mips.cc (right):

https://codereview.chromium.org/17421003/diff/24009/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:1500: __ addiu(SP, SP, Immediate(-5 * kWordSize));
On 2013/06/19 21:39:04, zra wrote:
> Only need 4 slots now.

Done.

https://codereview.chromium.org/17421003/diff/24009/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:1508: __ addiu(SP, SP, Immediate(5 * kWordSize)); 
// Discard argument;
On 2013/06/19 21:39:04, zra wrote:
> 4

Done.

https://codereview.chromium.org/17421003/diff/24009/runtime/vm/stub_code_mips...
runtime/vm/stub_code_mips.cc:1857: __ addiu(SP, SP, Immediate(-2 * kWordSize));
On 2013/06/19 21:39:04, zra wrote:
> Only need 1 slot now. If there's only one thing we're putting on the stack,
then
> Push and Pop are fine.

Done.

[siva, 2013-06-19 23:07:15]

lgtm

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_a...
File runtime/vm/code_patcher_arm_test.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_a...
runtime/vm/code_patcher_arm_test.cc:56: true, false, false, false, owner_class,
0));
I think function and function_name don't have to be zone handles here.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_a...
runtime/vm/code_patcher_arm_test.cc:60:
Array::ZoneHandle(ArgumentsDescriptor::New(1, Array::Handle()));
args_descriptor doesn't have to be a ZoneHandle anymore.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_i...
File runtime/vm/code_patcher_ia32_test.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_i...
runtime/vm/code_patcher_ia32_test.cc:60:
Array::ZoneHandle(ArgumentsDescriptor::New(1, Array::Handle()));
Ditto comment about ZoneHandle.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_arm.cc:913: ICData::New(function, name,
Array::Handle(),
use Object::null_array() instead of Array::Handle()

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_arm.cc:1099: ICData::New(function,
Symbols::Call(), Array::Handle(),
Ditto.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:1011: ICData::New(function, name,
Array::Handle(),
Ditto use Object::null_array()

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:1182: ICData::New(function,
Symbols::Call(), Array::Handle(),
Ditto.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_mips.cc:939: ICData::New(function, name,
Array::Handle(),
Ditto.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_mips.cc:1149: ICData::New(function,
Symbols::Call(), Array::Handle(),
Ditto.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_mips.cc:1345:
MegamorphicCache::ZoneHandle(table->Lookup(name, arguments_descriptor));
Some indentation issue here.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:1005: ICData::New(function, name,
Array::Handle(),
Ditto.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:1177: ICData::New(function,
Symbols::Call(), Array::Handle(),
Ditto.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:123: Array::Handle(),  // Dummy argument
descriptor.
Object::null_array() here and below.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:413: kNoArgumentNames));
Why does this have to be a ZoneHandle?

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:963: Array::ZoneHandle()));
Ditto question regarding ZoneHandle
and use Object::null_array() instead of Array::ZoneHandle()

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:362: kNoArgumentNames));
ZoneHandle not needed?

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1010: Array::ZoneHandle()));
ZoneHandle not needed and Object::null_array()

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_mips.cc:1022: Array::ZoneHandle()));
Ditto regarding ZoneHandle and Object::null_array()

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:518: kNoArgumentNames));
Ditto.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:1038: Array::ZoneHandle()));
ZoneHandle? and Object::null_array()

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

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/stub_code_x64....
runtime/vm/stub_code_x64.cc:1693: __ popq(R10);  // Preserve arguments
descriptor.
Shouldn't this be Restore in the comment.

[srdjan, 2013-06-20 00:12:34]

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_a...
File runtime/vm/code_patcher_arm_test.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_a...
runtime/vm/code_patcher_arm_test.cc:56: true, false, false, false, owner_class,
0));
On 2013/06/19 23:07:15, siva wrote:
> I think function and function_name don't have to be zone handles here.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_a...
runtime/vm/code_patcher_arm_test.cc:60:
Array::ZoneHandle(ArgumentsDescriptor::New(1, Array::Handle()));
On 2013/06/19 23:07:15, siva wrote:
> args_descriptor doesn't have to be a ZoneHandle anymore.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_i...
File runtime/vm/code_patcher_ia32_test.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/code_patcher_i...
runtime/vm/code_patcher_ia32_test.cc:60:
Array::ZoneHandle(ArgumentsDescriptor::New(1, Array::Handle()));
On 2013/06/19 23:07:15, siva wrote:
> Ditto comment about ZoneHandle.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_arm.cc:913: ICData::New(function, name,
Array::Handle(),
On 2013/06/19 23:07:15, siva wrote:
> use Object::null_array() instead of Array::Handle()

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_ia32.cc:1182: ICData::New(function,
Symbols::Call(), Array::Handle(),
On 2013/06/19 23:07:15, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_mips.cc:1149: ICData::New(function,
Symbols::Call(), Array::Handle(),
On 2013/06/19 23:07:15, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_mips.cc:1345:
MegamorphicCache::ZoneHandle(table->Lookup(name, arguments_descriptor));
On 2013/06/19 23:07:15, siva wrote:
> Some indentation issue here.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:1005: ICData::New(function, name,
Array::Handle(),
On 2013/06/19 23:07:15, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_com...
runtime/vm/flow_graph_compiler_x64.cc:1177: ICData::New(function,
Symbols::Call(), Array::Handle(),
On 2013/06/19 23:07:15, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_opt...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/flow_graph_opt...
runtime/vm/flow_graph_optimizer.cc:123: Array::Handle(),  // Dummy argument
descriptor.
On 2013/06/19 23:07:15, siva wrote:
> Object::null_array() here and below.

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:413: kNoArgumentNames));
On 2013/06/19 23:07:15, siva wrote:
> Why does this have to be a ZoneHandle?

Handle.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_arm.cc:963: Array::ZoneHandle()));
On 2013/06/19 23:07:15, siva wrote:
> Ditto question regarding ZoneHandle
> and use Object::null_array() instead of Array::ZoneHandle()

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:362: kNoArgumentNames));
On 2013/06/19 23:07:15, siva wrote:
> ZoneHandle not needed?

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_ia32.cc:1010: Array::ZoneHandle()));
On 2013/06/19 23:07:15, siva wrote:
> ZoneHandle not needed and Object::null_array()

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_mips.cc:1022: Array::ZoneHandle()));
On 2013/06/19 23:07:15, siva wrote:
> Ditto regarding ZoneHandle and Object::null_array()

Done.

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/intermediate_l...
runtime/vm/intermediate_language_x64.cc:518: kNoArgumentNames));
On 2013/06/19 23:07:15, siva wrote:
> Ditto.

Done.

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

https://codereview.chromium.org/17421003/diff/49001/runtime/vm/stub_code_x64....
runtime/vm/stub_code_x64.cc:1693: __ popq(R10);  // Preserve arguments
descriptor.
On 2013/06/19 23:07:15, siva wrote:
> Shouldn't this be Restore in the comment.

Done.

[srdjan, 2013-06-20 16:29:58]

Committed patchset #10 manually as r24239 (presubmit successful).

[Florian Schneider, 2013-06-21 10:29:26]

Please don't forget to cc vm-dev@ for all VM CLs.

Since the calling convention for IC calls changed, it also conflicted with my CL
for noSuchMethod dispatching.

[regis, 2013-06-24 18:12:24]

LGTM