Optimizations to IC stub for unoptimized code performance on x64.

Optimizations to IC stub for unoptimized code performance on x64.

Using two CMOVs for class ID load is not a win relative to a simple branch.
Unroll the loop that checks entries in the IC data to find correct call.
Don't check a counter for overflow on 64 bit, it's not going to happen and
even if it did it makes little difference.
Don't reload receiver and arg0 for every entry in the IC data in the
multidispatch case.
According to my measurements this IC stub is about 10% faster in the
single dispatch case, and about 15% faster in the less important double
dispatch case.  Unoptimzed code spends about 30% of its time in these
two stubs.
R=regis@google.com
BUG=

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

[regis, 2017-01-20 17:11:01]

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

[regis, 2017-01-20 17:11:02]

Most of the changes look good to me. But we try to keep the different
architectures in sync. Wouldn't any of these changes apply to other
architectures?

Also, I am not sure that removing the overflow check (which is also 32-bit even
on x64, btw) is safe. Disabling optimization is done with a -1 threshold, and
this may not work anymore without overflow check.

Florian, what do you think?

[Florian Schneider, 2017-01-20 17:26:35]

dbc:

Nice speedup! This should help really large apps like dart2js.

But I agree with Regis that we should have this on our mobile architectures
ARM/ARM64/MIPS as well. ia32 is not that important, but is almost a copy of x64
anyway.

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

https://codereview.chromium.org/2647913002/diff/1/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:1611: __ addq(Address(R12, count_offset),
Immediate(Smi::RawValue(1)));
I think this should be safe: We already use INT_MIN to mark functions should not
be optimized in the near future. Wrapping around here would only mean missing
optimization of that function, but does not affect correct execution.

[erikcorry, 2017-01-24 15:35:41]

I did the loop unroll on ARM, MIPS and IA32, too.

I did more measurements on how to load a class ID from something that may be a
Smi.  Turns out, a single CMOV (with a fake Smi object in memory) is faster on
IA32, whereas a branch is faster on x64.

In both cases the CMOV makes the stub slower, but using branches makes
everything else slower on IA32, probably because it is overloading the branch
predictor.

Golem results (when optimization is disabled)
x64:  6.5% faster
IA32: 7.1% faster
ARMv7: 1.3% faster
ARMv8: 1.7% faster

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

https://codereview.chromium.org/2647913002/diff/1/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:1611: __ addq(Address(R12, count_offset),
Immediate(Smi::RawValue(1)));
On 2017/01/20 17:26:35, Florian Schneider wrote:
> I think this should be safe: We already use INT_MIN to mark functions should
not
> be optimized in the near future. Wrapping around here would only mean missing
> optimization of that function, but does not affect correct execution.

I'll make this change on 32 bit too, then.  It is 64 bit here, btw (that's what
the 'q' means).

[regis, 2017-01-24 18:27:21]

lgtm

Thanks for improving all architectures!

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

https://codereview.chromium.org/2647913002/diff/1/runtime/vm/stub_code_x64.cc...
runtime/vm/stub_code_x64.cc:1611: __ addq(Address(R12, count_offset),
Immediate(Smi::RawValue(1)));
On 2017/01/24 15:35:41, erikcorry wrote:
> On 2017/01/20 17:26:35, Florian Schneider wrote:
> > I think this should be safe: We already use INT_MIN to mark functions should
> not
> > be optimized in the near future. Wrapping around here would only mean
missing
> > optimization of that function, but does not affect correct execution.
> 
> I'll make this change on 32 bit too, then.  It is 64 bit here, btw (that's
what
> the 'q' means).

Definitely a 64-bit overflow check. My bad. I got confused by the reset value
being in the 32-bit range.

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

https://codereview.chromium.org/2647913002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:13468: if (data_pos < 0) {
Can you explain with a comment how this is possible?

https://codereview.chromium.org/2647913002/diff/40001/runtime/vm/stub_code_ar...
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/2647913002/diff/40001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm.cc:1502: __ adds(R1, R1, Operand(Smi::RawValue(1)));
// Ignore overflow.

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

https://codereview.chromium.org/2647913002/diff/40001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:1426: __ Comment("Update caller's counter");
// Ignore overflow.

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

https://codereview.chromium.org/2647913002/diff/40001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:1311: // Update counter.
// Ignore overflow.

https://codereview.chromium.org/2647913002/diff/40001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:1462: __ Comment("Update caller's counter");
// Ignore overflow.

https://codereview.chromium.org/2647913002/diff/40001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:1605: // Increment count for this call.
// Ignore overflow.

[erikcorry, 2017-01-25 09:15:10]

Description was changed from

==========
Optimizations to IC stub for unoptimized code performance on x64.

Using two CMOVs for class ID load is not a win relative to a simple branch.
Unroll the loop that checks entries in the IC data to find correct call.
Don't check a counter for overflow on 64 bit, it's not going to happen and
even if it did it makes little difference.
Don't reload receiver and arg0 for every entry in the IC data in the
multidispatch case.
According to my measurements this IC stub is about 10% faster in the
single dispatch case, and about 15% faster in the less important double
dispatch case.  Unoptimzed code spends about 30% of its time in these
two stubs.
R=regis@google.com
BUG=
==========

to

==========
Optimizations to IC stub for unoptimized code performance on x64.

Using two CMOVs for class ID load is not a win relative to a simple branch.
Unroll the loop that checks entries in the IC data to find correct call.
Don't check a counter for overflow on 64 bit, it's not going to happen and
even if it did it makes little difference.
Don't reload receiver and arg0 for every entry in the IC data in the
multidispatch case.
According to my measurements this IC stub is about 10% faster in the
single dispatch case, and about 15% faster in the less important double
dispatch case.  Unoptimzed code spends about 30% of its time in these
two stubs.
R=regis@google.com
BUG=

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

[erikcorry, 2017-01-25 09:15:11]

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