Precompilation: Generate instance calls as IC calls that can switch to Megamoprhic calls.

Precompilation: Generate instance calls as IC calls that can switch to Megamoprhic calls.

dart2js ARM -10.3% size

R=fschneider@google.com

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

[rmacnak, 2015-11-03 21:51:56]

Description was changed from

==========
Precompilation: Generate instance calls as IC calls that can switch to
Megamoprhic calls.

BUG=
==========

to

==========
Precompilation: Generate instance calls as IC calls that can switch to
Megamoprhic calls.

BUG=
==========

[rmacnak, 2015-11-03 22:08:19]

rmacnak@google.com changed reviewers:
+ fschneider@google.com

[srdjan, 2015-11-03 22:30:48]

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

[srdjan, 2015-11-03 22:30:50]

DBC

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1015: //   Arg1: ICData object.
or cache

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1023: String& name = String::Handle();
zone is already defined here, please use it for handles.

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1054: if (ic_data.NumberOfChecks() ==
FLAG_max_polymorphic_checks) {
Use '>=', it is more reliable than '=='

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1084: // Arg1: IC data
or cache

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_patche...
File runtime/vm/code_patcher_ia32.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_patche...
runtime/vm/code_patcher_ia32.cc:219: UNREACHABLE();
Please add a comment why,

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_patche...
File runtime/vm/code_patcher_x64.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_patche...
runtime/vm/code_patcher_x64.cc:187: class SwitchableCall : public ValueObject {
Add a comment before class definition.

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/stub_code_a...
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/stub_code_a...
runtime/vm/stub_code_arm.cc:2103: //  R9: ICData (preserved)
cache?

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/stub_code_a...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/stub_code_a...
runtime/vm/stub_code_arm64.cc:2157: //  R5: ICData (preserved)
/cache

[rmacnak, 2015-11-03 23:54:30]

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1015: //   Arg1: ICData object.
On 2015/11/03 22:30:49, srdjan wrote:
> or cache

Done.

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1023: String& name = String::Handle();
On 2015/11/03 22:30:49, srdjan wrote:
> zone is already defined here, please use it for handles.

Done

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1054: if (ic_data.NumberOfChecks() ==
FLAG_max_polymorphic_checks) {
On 2015/11/03 22:30:49, srdjan wrote:
> Use '>=', it is more reliable than '=='

Done.

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1084: // Arg1: IC data
On 2015/11/03 22:30:49, srdjan wrote:
> or cache

Done.

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_patche...
File runtime/vm/code_patcher_ia32.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_patche...
runtime/vm/code_patcher_ia32.cc:219: UNREACHABLE();
On 2015/11/03 22:30:49, srdjan wrote:
> Please add a comment why,

Added

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_patche...
File runtime/vm/code_patcher_x64.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/code_patche...
runtime/vm/code_patcher_x64.cc:187: class SwitchableCall : public ValueObject {
On 2015/11/03 22:30:49, srdjan wrote:
> Add a comment before class definition.

Done.

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/stub_code_a...
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/stub_code_a...
runtime/vm/stub_code_arm.cc:2103: //  R9: ICData (preserved)
On 2015/11/03 22:30:49, srdjan wrote:
> cache?

only ICData

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/stub_code_a...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/1418863003/diff/180001/runtime/vm/stub_code_a...
runtime/vm/stub_code_arm64.cc:2157: //  R5: ICData (preserved)
On 2015/11/03 22:30:49, srdjan wrote:
> /cache

only ICData

[Florian Schneider, 2015-11-04 01:27:10]

LGTM

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/code_genera...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1066: code, ic_data, cache, stub);
I think target_function should be inserted in the cache here as well.

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_ia32.cc:1314: void
FlowGraphCompiler::EmitSwitchableInstanceCall(
Is this invoked at all on ia32?

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/instruction...
File runtime/vm/instructions_arm64.cc (right):

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/instruction...
runtime/vm/instructions_arm64.cc:337: // Last instruction: blr x1.
s/x1/r1/

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/stub_code_x...
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/stub_code_x...
runtime/vm/stub_code_x64.cc:2165: __ cmpq(R9,
Immediate(Smi::RawValue(kIllegalCid)));
Alternatively: testq(R9, R9)
plus adding the assert that kIllegalCid == 0.

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/stub_code_x...
runtime/vm/stub_code_x64.cc:2181: __ movq(RAX, Address(RAX,
Isolate::ic_miss_function_offset()));
Would it work to just store the code object
StubCode::MegamorphicMiss_entry()->code() in the Isolate or Thread? (like we do
it for the StoreBufferUpdate code)

Then you would not need to load the miss-function here.

[rmacnak, 2015-11-04 01:50:22]

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/code_genera...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/code_genera...
runtime/vm/code_generator.cc:1066: code, ic_data, cache, stub);
On 2015/11/04 01:27:10, Florian Schneider wrote:
> I think target_function should be inserted in the cache here as well.

Done.

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/instruction...
File runtime/vm/instructions_arm64.cc (right):

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/instruction...
runtime/vm/instructions_arm64.cc:337: // Last instruction: blr x1.
On 2015/11/04 01:27:10, Florian Schneider wrote:
> s/x1/r1/

Done.

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/stub_code_x...
File runtime/vm/stub_code_x64.cc (right):

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/stub_code_x...
runtime/vm/stub_code_x64.cc:2165: __ cmpq(R9,
Immediate(Smi::RawValue(kIllegalCid)));
On 2015/11/04 01:27:10, Florian Schneider wrote:
> Alternatively: testq(R9, R9)
> plus adding the assert that kIllegalCid == 0.

Done.

https://codereview.chromium.org/1418863003/diff/200001/runtime/vm/stub_code_x...
runtime/vm/stub_code_x64.cc:2181: __ movq(RAX, Address(RAX,
Isolate::ic_miss_function_offset()));
On 2015/11/04 01:27:10, Florian Schneider wrote:
> Would it work to just store the code object
> StubCode::MegamorphicMiss_entry()->code() in the Isolate or Thread? (like we
do
> it for the StoreBufferUpdate code)
> 
> Then you would not need to load the miss-function here.

It's not in the VM isolate so we can't use Thread. But we can load the code
directly instead of through the function.

[rmacnak, 2015-11-04 17:31:24]

Committed patchset #12 (id:220001) manually as
830bfe61b725247e1fb5138f3ab7b129a222b68d.

[rmacnak, 2015-11-05 18:20:42]

On 2015/11/04 17:31:24, rmacnak wrote:
> Committed patchset #12 (id:220001) manually as
> 830bfe61b725247e1fb5138f3ab7b129a222b68d.

Golem dart-unopt

DeltaBlueClosures (ARMv7HF) -1.030%
Tracer (ARMv7HF) -0.353%
CompileOnceHelloHtml (ARMv7HF) 9.501%
Richards (ARMv7HF) 11.387%
DeltaBlue (ARMv7HF) 13.066%
ParticleSystemUpdate (ARMv7HF) 27.645%
FluidMotion (ARMv7HF) 34.904%
ParticleSystemPaint (ARMv7HF) 68.669%

[rmacnak, 2015-11-10 02:22:29]

On 2015/11/05 18:20:42, rmacnak wrote:
> On 2015/11/04 17:31:24, rmacnak wrote:
> > Committed patchset #12 (id:220001) manually as
> > 830bfe61b725247e1fb5138f3ab7b129a222b68d.
> 
> Golem dart-unopt
> 
> DeltaBlueClosures (ARMv7HF) -1.030%
> Tracer (ARMv7HF) -0.353%
> CompileOnceHelloHtml (ARMv7HF) 9.501%
> Richards (ARMv7HF) 11.387%
> DeltaBlue (ARMv7HF) 13.066%
> ParticleSystemUpdate (ARMv7HF) 27.645%
> FluidMotion (ARMv7HF) 34.904%
> ParticleSystemPaint (ARMv7HF) 68.669%


Adding fast Smi ops didn't improve things.

https://chromiumcodereview.appspot.com/1429303005/

http://146.148.43.81:8080/Comparison#targetA%3Ddart-unopt%3BmachineTypeA%3Dli...

[kasperl, 2015-11-10 08:39:46]

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

[kasperl, 2015-11-10 08:39:46]

Very nice progress!