Faster IC stubs by specializing them for Binary Smi operations

Faster IC stubs by specializing them for two-argument Smi operations (implemented only a few for now)

R=zra@google.com

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

[Cutch, 2014-08-15 21:23:28]

DBC

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2444: if (label_address != 0) {
!= NULL ?

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11380: String::Handle(target.name()).ToCString(),
Should this be an ASSERT?

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11688: // TODO(srdjan): Make this test more robust.
What about factoring this check into:

bool IsUnused(intptr_t i) const;

And then using it below in FilterUnusedEntries for consistency.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1317: // Note: xxx must be preserved if
'not_smi_or_overflow' is jumped to.
xxx?

[Ivan Posva, 2014-08-15 21:32:35]

-ip

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2444: if (label_address != 0) {
On 2014/08/15 21:23:28, Cutch wrote:
> != NULL ?

uword is an integer type.

[Vyacheslav Egorov (Google), 2014-08-15 21:36:19]

I feel like we are going the path that we walked already and we know that it is
painful: duplication is not beneficial for maintainability. I wonder if there is
a way to achieve this without duplicating this code in yet another place (we
already have it in the intricifier and the intermediate language right?)

[zra, 2014-08-15 21:45:09]

Is there some way to do this for all intrinsics?

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11713: if (count <= 0) continue;
Above, GetCountAt(i) is checked for equality with zero. Why is it different
here? Also, please use curly braces around continue;

Above, there are also a checks for NumArgsTest() > 0 and on the cid. Why don't
you need to do those here, too? Couldn't you be filtering too many things?

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm.cc:1247: case Token::kEQ: {
Use conditional ops:

__ cmp(R0, Operand(R1));
__ LoadObject(R0, Bool::True(), EQ);
__ LoadObject(R0, Bool::False(), NE);

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm.cc:1275: __ Bind(&error);
If it's impossible to recover after incorrect IC data, then maybe move this
after the Ret(), and get rid of the "ok" branch and label.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1317: // Note: xxx must be preserved if
'not_smi_or_overflow' is jumped to.
On 2014/08/15 21:23:28, Cutch wrote:
> xxx?

Should be R5 as on ARM.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1346: Label done, is_true;
Using conditional select:

__ CompareRegisters(R0, R1);
__ LoadObject(R0, Bool::True(), PP);
__ LoadObject(R1, Bool::False(), PP);
__ csel(R0, R1, R0, NE);

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1374: __ Bind(&error);
Same comment as on ARM.

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:1318: __ Bind(&error);
Same comment as on ARM.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_mi...
File runtime/vm/stub_code_mips.cc (right):

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_mi...
runtime/vm/stub_code_mips.cc:1425: __ Bind(&error);
Same comment as on ARM.

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_x6...
runtime/vm/stub_code_x64.cc:1297: __ Bind(&error);
Same comment as on ARM.

[srdjan, 2014-08-15 21:49:00]

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11380: String::Handle(target.name()).ToCString(),
On 2014/08/15 21:23:28, Cutch wrote:
> Should this be an ASSERT?

This is gone, forgot to upload after removing it. Sorry. The assert is below,
this was debugging code as I could not print from LLDB

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11688: // TODO(srdjan): Make this test more robust.
On 2014/08/15 21:23:28, Cutch wrote:
> What about factoring this check into:
> 
> bool IsUnused(intptr_t i) const;
> 
> And then using it below in FilterUnusedEntries for consistency.

FilterUnusedEntries currently always creates a new ICData, I would like to do
that only if needed, i.e., I need first one pass to determine if allocation and
copying is needed at all.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1317: // Note: xxx must be preserved if
'not_smi_or_overflow' is jumped to.
On 2014/08/15 21:23:28, Cutch wrote:
> xxx?

R6.

[zra, 2014-08-15 22:12:33]

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1317: // Note: xxx must be preserved if
'not_smi_or_overflow' is jumped to.
On 2014/08/15 21:49:00, srdjan wrote:
> On 2014/08/15 21:23:28, Cutch wrote:
> > xxx?
> 
> R6.

Ah, right, sorry.

[srdjan, 2014-08-18 20:28:40]

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11688: // TODO(srdjan): Make this test more robust.
On 2014/08/15 21:23:28, Cutch wrote:
> What about factoring this check into:
> 
> bool IsUnused(intptr_t i) const;
> 
> And then using it below in FilterUnusedEntries for consistency.

Discussed offline, misunderstood the comment.
Done.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11713: if (count <= 0) continue;
On 2014/08/15 21:45:08, zra wrote:
> Above, GetCountAt(i) is checked for equality with zero. Why is it different
> here? Also, please use curly braces around continue;
> 
> Above, there are also a checks for NumArgsTest() > 0 and on the cid. Why don't
> you need to do those here, too? Couldn't you be filtering too many things?

Done using Cutch's suggestion.

[srdjan, 2014-08-18 20:40:27]

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm.cc:1247: case Token::kEQ: {
On 2014/08/15 21:45:08, zra wrote:
> Use conditional ops:
> 
> __ cmp(R0, Operand(R1));
> __ LoadObject(R0, Bool::True(), EQ);
> __ LoadObject(R0, Bool::False(), NE);

Done.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm.cc:1275: __ Bind(&error);
On 2014/08/15 21:45:08, zra wrote:
> If it's impossible to recover after incorrect IC data, then maybe move this
> after the Ret(), and get rid of the "ok" branch and label.

I would need to have two If defined(DEBUG), the performance of this code is not
that significant and the code seems more readable this way. Let me know if you
disagree.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
File runtime/vm/stub_code_arm64.cc (right):

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1317: // Note: xxx must be preserved if
'not_smi_or_overflow' is jumped to.
On 2014/08/15 22:12:33, zra wrote:
> On 2014/08/15 21:49:00, srdjan wrote:
> > On 2014/08/15 21:23:28, Cutch wrote:
> > > xxx?
> > 
> > R6.
> 
> Ah, right, sorry.

You are right: R5 :-).

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1346: Label done, is_true;
On 2014/08/15 21:45:08, zra wrote:
> Using conditional select:
> 
> __ CompareRegisters(R0, R1);
> __ LoadObject(R0, Bool::True(), PP);
> __ LoadObject(R1, Bool::False(), PP);
> __ csel(R0, R1, R0, NE);

Done.

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm64.cc:1374: __ Bind(&error);
On 2014/08/15 21:45:08, zra wrote:
> Same comment as on ARM.

Ditto.

[zra, 2014-08-18 21:00:30]

lgtm

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

https://codereview.chromium.org/436643002/diff/240001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm.cc:1275: __ Bind(&error);
On 2014/08/18 20:40:27, srdjan wrote:
> On 2014/08/15 21:45:08, zra wrote:
> > If it's impossible to recover after incorrect IC data, then maybe move this
> > after the Ret(), and get rid of the "ok" branch and label.
> 
> I would need to have two If defined(DEBUG), the performance of this code is
not
> that significant and the code seems more readable this way. Let me know if you
> disagree.

Ah, okay. Missed that this was debug-only.

[srdjan, 2014-08-20 18:11:05]

PTAL. Changed handling of ICData with unused entries. Instead of replacing them
with copies that exclude unused entries, detect unused entries as we analyze
ICData. That way the original ICData can get updated with type feedback even in
optimized code. Verified performance.
Added flags:
--print-ic-data-map : debuggging
--two-args-smi-icd : enables this CL

[zra, 2014-08-20 20:17:11]

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_ia32.cc:1485: ASSERT(!ic_data.IsNull() &&
(ic_data.NumberOfUsedChecks() > 0));
Is this change also needed by the other architectures?

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:850: static bool
ICDataHasReceiverArgumentClassIds(const ICData& ic_data,
Could this function be implemented by calling
ICDataHasOnlyReveiverArgumentClassIds?

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:878: for (intptr_t i = 0; i <
class_ids.length(); i++) {
maybe ASSERT(class_ids[i] != kIllegalCid); to match
ICDataHasReveiverArgumentClassIds

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2481: }
Not sure if it makes sense to have an assertion here on NumberOfChecks or
NumberOfUsedChecks to make sure they produce the expected results after you've
just modified call_ic_data.

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

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11708: 
Extra newline.

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

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/object.h#new...
runtime/vm/object.h:3491: // Returns only used entries.,
remove comma.

[srdjan, 2014-08-21 17:30:22]

Thanks for comments and PTAL.

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_ia32.cc:1485: ASSERT(!ic_data.IsNull() &&
(ic_data.NumberOfUsedChecks() > 0));
On 2014/08/20 20:17:10, zra wrote:
> Is this change also needed by the other architectures?

Done on all architectures.

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:850: static bool
ICDataHasReceiverArgumentClassIds(const ICData& ic_data,
On 2014/08/20 20:17:10, zra wrote:
> Could this function be implemented by calling
> ICDataHasOnlyReveiverArgumentClassIds?

Done.

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:878: for (intptr_t i = 0; i <
class_ids.length(); i++) {
On 2014/08/20 20:17:10, zra wrote:
> maybe ASSERT(class_ids[i] != kIllegalCid); to match
> ICDataHasReveiverArgumentClassIds

Done.

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2481: }
On 2014/08/20 20:17:10, zra wrote:
> Not sure if it makes sense to have an assertion here on NumberOfChecks or
> NumberOfUsedChecks to make sure they produce the expected results after you've
> just modified call_ic_data.

I modify IC Data only if there are no previous checks. I do not think asserts
would help. Please ping me if I misunderstood your suggestion.

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

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11708: 
On 2014/08/20 20:17:10, zra wrote:
> Extra newline.

removed

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

https://codereview.chromium.org/436643002/diff/460001/runtime/vm/object.h#new...
runtime/vm/object.h:3491: // Returns only used entries.,
On 2014/08/20 20:17:11, zra wrote:
> remove comma.

Done.

[zra, 2014-08-21 17:41:52]

lgtm

[srdjan, 2014-08-21 20:08:54]

Committed patchset #25 manually as r39471 (presubmit successful).

[Florian Schneider, 2014-09-18 12:32:28]

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

[Florian Schneider, 2014-09-18 12:32:30]

https://codereview.chromium.org/436643002/diff/480001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/436643002/diff/480001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:5201: if
(ic_data().NumberOfUsedChecks() == 0) {
Why is this only changed on ia32? This also seems to be unreachable code.

[srdjan, 2014-09-25 17:40:25]

https://codereview.chromium.org/436643002/diff/480001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/436643002/diff/480001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:5201: if
(ic_data().NumberOfUsedChecks() == 0) {
On 2014/09/18 12:32:29, Florian Schneider wrote:
> Why is this only changed on ia32? This also seems to be unreachable code.

This was fixed in a follow up CL