Faster checks in polymorphic instance calls if Smi-s are involved

Faster checks in polymorphic instance calls if Smi-s are involved


BUG=
R=zra@google.com

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

[srdjan, 2015-06-15 19:49:16]

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

[zra, 2015-06-15 20:17:23]

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

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1561: // It is Smi, butSmi is not handled
here.
butSmi -> but Smi

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1562: __ b(failed, EQ);
Why isn't this:

if (kNumChecks == 1) {
  __ b(failed, EQ);
} else { ... }

as above?

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1572: if (kSortedLen == 0) return;
Should this branch to the 'failed' label?

If the branch to failed on 1562 doesn't need to be rewritten as in my comment
there, then maybe this should be an ASSERT instead.

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

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_ia32.cc:1534: Label next_test;
'Label after_smi_test' as in other arches to avoid name conflict with next_test
below.

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_ia32.cc:1575: assembler()->cmpl(EDI,
Immediate(sorted[i].cid));
__ is used above for assembler()->. Is it available here?

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

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_x64.cc:1498: Label next_test;
after_smi_test

[srdjan, 2015-06-15 20:54:13]

PTAL

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

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1561: // It is Smi, butSmi is not handled
here.
On 2015/06/15 20:17:22, zra wrote:
> butSmi -> but Smi

Done.

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1562: __ b(failed, EQ);
On 2015/06/15 20:17:22, zra wrote:
> Why isn't this:
> 
> if (kNumChecks == 1) {
>   __ b(failed, EQ);
> } else { ... }
> 
> as above?

Added/changed comment:

    // Receiver is Smi, but Smi is not a valid class therefore fail.
    // (Smi class must be first in the list).

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1572: if (kSortedLen == 0) return;
On 2015/06/15 20:17:23, zra wrote:
> Should this branch to the 'failed' label?
> 
> If the branch to failed on 1562 doesn't need to be rewritten as in my comment
> there, then maybe this should be an ASSERT instead.

Added comment:

  // If kSortedLen is 0 then only a Smi check was needed; the Smi check above
  // will fail if there was only one check and receiver is not Smi.

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1577: Label next_test;
Added assert that sorted[i].cid is not kSMiCid

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

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_ia32.cc:1534: Label next_test;
On 2015/06/15 20:17:23, zra wrote:
> 'Label after_smi_test' as in other arches to avoid name conflict with
next_test
> below.

Changed to after_smi_test

https://codereview.chromium.org/1184093004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_ia32.cc:1575: assembler()->cmpl(EDI,
Immediate(sorted[i].cid));
On 2015/06/15 20:17:23, zra wrote:
> __ is used above for assembler()->. Is it available here?

Yes, replaced with __

[zra, 2015-06-15 21:01:47]

lgtm

[srdjan, 2015-06-15 21:54:49]

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