Cleanup: Make CheckClassId instruction more general so it

Cleanup: Make CheckClassId instruction more general so it
can be used uniformly in the polymorphic inliner.

R=kustermann@google.com
BUG=

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

[kustermann, 2017-05-18 09:59:45]

LGTM

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/constants_dbc.h
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/constants_dbc.h#...
runtime/vm/constants_dbc.h:849: V(CheckClassIdRange,                 A_B_C, reg,
num, num) \
The class range [start, end] limits are constrained to be in [0, 255] due to the
4-byte bytecode encoding. Is it anywhere enforced that we will never use the
cid-range chedck instructions if we have more than 255 classes?

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

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:1064: name.ToCString());
Consider either removing the name in this case, or print the name for both ends
of the interval (i.e. "cids().cid_start" and "cids().cid_end")

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:565: intptr_t cid_end;
Maybe consider removing the "cid_" prefixes, it's implied by the CidRange
struct.

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:6420: __ CompareImmediate(value,
Smi::RawValue(cids_.cid_end - cids_.cid_start));
Add a "length" / "size" getter to Cids?

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:1499:
compiler->ToEmbeddableCid(cids_.cid_end, this));
Please ASSERT that the range is within [0, 255] here.

Is it guaranteed by construction of the CheckClassId instruction?

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

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:3263: pc += (actual_cid - cid_start <= cid_end + 1u
- cid_start) ? 1 : 0;
If

  [cid_start, cid_end] = [0, 1]
  actual_cid = 2

This translates to

  2 - 0 <= 1 + 1 - 0
  2     <= 2

despite actual_cid not in [cid_start, cid_end]!

[erikcorry, 2017-05-18 14:25:30]

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/constants_dbc.h
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/constants_dbc.h#...
runtime/vm/constants_dbc.h:849: V(CheckClassIdRange,                 A_B_C, reg,
num, num) \
On 2017/05/18 09:59:45, kustermann wrote:
> The class range [start, end] limits are constrained to be in [0, 255] due to
the
> 4-byte bytecode encoding. Is it anywhere enforced that we will never use the
> cid-range chedck instructions if we have more than 255 classes?

Changed to a 16 bit representation with the 2nd argument in a following Nop
instruction.

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

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:1064: name.ToCString());
On 2017/05/18 09:59:45, kustermann wrote:
> Consider either removing the name in this case, or print the name for both
ends
> of the interval (i.e. "cids().cid_start" and "cids().cid_end")

Done.

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:565: intptr_t cid_end;
On 2017/05/18 09:59:45, kustermann wrote:
> Maybe consider removing the "cid_" prefixes, it's implied by the CidRange
> struct.

I took a look at this, and the word "CidRange" is not usually visible near the
word cid_start or cid_end, so I think this would make the code harder to read.

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:6420: __ CompareImmediate(value,
Smi::RawValue(cids_.cid_end - cids_.cid_start));
On 2017/05/18 09:59:45, kustermann wrote:
> Add a "length" / "size" getter to Cids?

I called it "Extent()"

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:1499:
compiler->ToEmbeddableCid(cids_.cid_end, this));
On 2017/05/18 09:59:45, kustermann wrote:
> Please ASSERT that the range is within [0, 255] here.
> 
> Is it guaranteed by construction of the CheckClassId instruction?

Nice catch, this was wrong, fixed with a double DBC instruction.

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

https://codereview.chromium.org/2891713002/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:3263: pc += (actual_cid - cid_start <= cid_end + 1u
- cid_start) ? 1 : 0;
On 2017/05/18 09:59:45, kustermann wrote:
> If
> 
>   [cid_start, cid_end] = [0, 1]
>   actual_cid = 2
> 
> This translates to
> 
>   2 - 0 <= 1 + 1 - 0
>   2     <= 2
> 
> despite actual_cid not in [cid_start, cid_end]!

Oops, fixed.

[erikcorry, 2017-05-19 06:50:16]

Description was changed from

==========
Cleanup: Make CheckClassId instruction more general so it
can be used uniformly in the polymorphic inliner.

R=kustermann@google.com
BUG=
==========

to

==========
Cleanup: Make CheckClassId instruction more general so it
can be used uniformly in the polymorphic inliner.

R=kustermann@google.com
BUG=

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

[erikcorry, 2017-05-19 06:50:48]

Committed patchset #2 (id:20001) manually as
4aa5e1d7aa769f69515ecb37a9cd6260c218793a (presubmit successful).