Use off-heap data for class check instructions

Use off-heap data for class check instructions

This continues the work from https://codereview.chromium.org/2842753002/
- see the motivation there.

R=vegorov@google.com
BUG=

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

[Vyacheslav Egorov (Google), 2017-05-01 12:21:26]

first round of comments.

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:465: *CallTargets::Create(Z, *call->ic_data(), 0),
call->deopt_id(),
please add a comment to the numeric argument here and everywhere, e.g. 

CallTargets::Create(Z, ..., /*argument_number=*/ 0)

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:490: if (!call->ic_data()->NumberOfChecksIs(1)) {
these are not equivalent - as unary checks collapses 2 checks [(x, y), (x, z)]
to 1 unary check [(x)]. If this does not matter please explain why here.

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:565: AddCheckClass(right, *targets_1,
call->deopt_id(), call->env(), call);
This code is repeated, so it could be a helper:

AddCheckClassForArgNr(right, call->ic_data(), /*argnr=*/1, call->deopt_id(),
call->env(), call);

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/flow_graph.h#new...
runtime/vm/flow_graph.h:136: Instruction* GetCheckClass(Definition* to_check,
I think this name is confusing. Maybe MakeCheckClass(...)?

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/flow_graph_inlin...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:2695: value_check = new (Z) CallTargets();
Maybe make a helper CallTargets::CreateMonomorphic(...)

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:171: bool
CallTargets::AreAllChecksImmutable() const {
this name is very confusing. maybe rename it to ContainsExternalizableCids(...)

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:184: bool
CallTargets::HasReceiverClassId(intptr_t cid) const {
I think this is the same as HasClassId()?

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:262: if (min <= kSmiCid && kSmiCid <= max)
return false;
this is not equivalent: it is fine to have kSmiCid between min and max as long
as it is not actually *contained* in the targets (i.e. not all cids between min
and max are contained in the targets).

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:297: intptr_t run = (1 << (1 + cid_end -
cid_start)) - 1;
I don't think this works if (cid_end - cid_start + 1) = kBitsPerWord

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:574: class CallTargets : public ZoneAllocated
{
It feels a bit strange to use CallTargets as a "cid-list" in the ClassCheck. 

There are a lot of information and functionality on this class that make no
sense when it is used for checks and similarly there is information and
functionality that only makes sense for checks.

Maybe we need to split this into two classes now:

// Contains a list of cid-ranges (potentially with frequences, maybe not)
class CidRangeList {
};

// Contains a mapping from cid-ranges to functions.
class CallTargets {
  
};

Now here is an interesting question: whether CallTargets should inherit from
CidRangeList or it should just contain it inside. I think composition is better.

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:6332: void
CheckClassInstr::EmitDenseSwitch(FlowGraphCompiler* compiler,
I think original name DenseSwitch is confusing. there is nothing really dense
about it. Maybe

EmitBitTest()?

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:6337: Register biased_cid =
locs()->temp(0).reg();
it's bit confusing to name it biased_cid because it is not biased before this
instruction.

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:6342: Register mask_reg =
locs()->temp(1).reg();
this should probably be called bit_reg, because it contains a single bit, rather
than mask.

Also I wonder if instead of (1 << biased_cid) & mask, it's not better to do
(mask >> biased_cid) & 1 - might be a shorter instruction sequence.

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_ia32.cc:5864: __ bt(mask_reg, biased_cid);
I wonder if bt() is even good on modern hardware

[erikcorry, 2017-05-05 15:15:52]

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:465: *CallTargets::Create(Z, *call->ic_data(), 0),
call->deopt_id(),
On 2017/05/01 12:21:24, Vyacheslav Egorov (Google) wrote:
> please add a comment to the numeric argument here and everywhere, e.g. 
> 
> CallTargets::Create(Z, ..., /*argument_number=*/ 0)

Done.

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:490: if (!call->ic_data()->NumberOfChecksIs(1)) {
On 2017/05/01 12:21:24, Vyacheslav Egorov (Google) wrote:
> these are not equivalent - as unary checks collapses 2 checks [(x, y), (x, z)]
> to 1 unary check [(x)]. If this does not matter please explain why here.
>  

Good catch, it does matter.

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:565: AddCheckClass(right, *targets_1,
call->deopt_id(), call->env(), call);
On 2017/05/01 12:21:24, Vyacheslav Egorov (Google) wrote:
> This code is repeated, so it could be a helper:
> 
> AddCheckClassForArgNr(right, call->ic_data(), /*argnr=*/1, call->deopt_id(),
> call->env(), call);
> 

Done.

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/flow_graph.h#new...
runtime/vm/flow_graph.h:136: Instruction* GetCheckClass(Definition* to_check,
On 2017/05/01 12:21:24, Vyacheslav Egorov (Google) wrote:
> I think this name is confusing. Maybe MakeCheckClass(...)?

Changed to CreateCheckClass

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/flow_graph_inlin...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:2695: value_check = new (Z) CallTargets();
On 2017/05/01 12:21:24, Vyacheslav Egorov (Google) wrote:
> Maybe make a helper CallTargets::CreateMonomorphic(...)

Done.

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:171: bool
CallTargets::AreAllChecksImmutable() const {
On 2017/05/01 12:21:25, Vyacheslav Egorov (Google) wrote:
> this name is very confusing. maybe rename it to
ContainsExternalizableCids(...)

Done.

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:184: bool
CallTargets::HasReceiverClassId(intptr_t cid) const {
On 2017/05/01 12:21:25, Vyacheslav Egorov (Google) wrote:
> I think this is the same as HasClassId()?

Merged

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:262: if (min <= kSmiCid && kSmiCid <= max)
return false;
On 2017/05/01 12:21:25, Vyacheslav Egorov (Google) wrote:
> this is not equivalent: it is fine to have kSmiCid between min and max as long
> as it is not actually *contained* in the targets (i.e. not all cids between
min
> and max are contained in the targets).
> 

Done.

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:297: intptr_t run = (1 << (1 + cid_end -
cid_start)) - 1;
On 2017/05/01 12:21:24, Vyacheslav Egorov (Google) wrote:
> I don't think this works if (cid_end - cid_start + 1) = kBitsPerWord

Done.

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:574: class CallTargets : public ZoneAllocated
{
On 2017/05/01 12:21:25, Vyacheslav Egorov (Google) wrote:
> It feels a bit strange to use CallTargets as a "cid-list" in the ClassCheck. 
> 
> There are a lot of information and functionality on this class that make no
> sense when it is used for checks and similarly there is information and
> functionality that only makes sense for checks.
> 
> Maybe we need to split this into two classes now:
> 
> // Contains a list of cid-ranges (potentially with frequences, maybe not)
> class CidRangeList {
> };
> 
> // Contains a mapping from cid-ranges to functions.
> class CallTargets {
>   
> };
> 
> Now here is an interesting question: whether CallTargets should inherit from
> CidRangeList or it should just contain it inside. I think composition is
better.
> 

I tried several things here.  In the end the one that worked best was to have a
class called Cids for class checks.  CallTargets is used for polymorphic
dispatch and inherits from Cids. They both have a growable array of pointers to
CidRange objects (just a cid pair), but in the case of CallTargets the elements
are actually pointing to instances of CallInfo, a subclass of CidRange that
contains target and count information.

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:6332: void
CheckClassInstr::EmitDenseSwitch(FlowGraphCompiler* compiler,
On 2017/05/01 12:21:25, Vyacheslav Egorov (Google) wrote:
> I think original name DenseSwitch is confusing. there is nothing really dense
> about it. Maybe
> 
> EmitBitTest()?

Done.

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:6337: Register biased_cid =
locs()->temp(0).reg();
On 2017/05/01 12:21:25, Vyacheslav Egorov (Google) wrote:
> it's bit confusing to name it biased_cid because it is not biased before this
> instruction.

As discussed offline the bias is initially zero.  We can't easily change the
name of the register although it contains different values during this function,
so I'm leaving it as is.

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:6342: Register mask_reg =
locs()->temp(1).reg();
On 2017/05/01 12:21:25, Vyacheslav Egorov (Google) wrote:
> this should probably be called bit_reg, because it contains a single bit,
rather
> than mask.
> 
> Also I wonder if instead of (1 << biased_cid) & mask, it's not better to do
> (mask >> biased_cid) & 1 - might be a shorter instruction sequence.

Done.

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

https://codereview.chromium.org/2856543002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_ia32.cc:5864: __ bt(mask_reg, biased_cid);
On 2017/05/01 12:21:25, Vyacheslav Egorov (Google) wrote:
> I wonder if bt() is even good on modern hardware

According to Agner Fog it's pretty good.

[Vyacheslav Egorov (Google), 2017-05-09 21:07:28]

lgtm

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:2112: const Cids* cids = Cids::Create(Z, ic_data,
0);
please add a comment here about what does 0 means.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/aot_optimizer.h
File runtime/vm/aot_optimizer.h (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.h:96: void AddChecksToArgNr(InstanceCallInstr* call,
s/ToArgNr/ForArgNr/ ?

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.h (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:1172: void AddImmediate(Register dest, int64_t
imm);
It does not seem like anybody is actually using this function.

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

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:171: bool Cids::ContainsExternalizableCids()
const {
How about moving Cids methods all together instead of interspersing them with
methods of other classes?

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:280: if (1ul + cid_end - cid_start >=
sizeof(intptr_t) * 8) {
there is kBitsPerWord

make a variable for cid_end - cid_start + 1 and use it here and below.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2769: const TargetInfo* target_info_a =
reinterpret_cast<const TargetInfo*>(*a);
static_cast instead.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2902: if (cid_ranges_[dest]->cid_end + 1 >=
cid_ranges_[src]->cid_start &&
for uniformity use TargetAt(x) instead of cid_ranges_[x] consistently in this
function.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:558: CidRange(const CidRange& o)
Please order constructors before fields.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:562: CidRange(intptr_t cid_start_arg,
I don't think this constructor is used.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:650: return
reinterpret_cast<TargetInfo*>(At(i));
static_cast<TargetInfo*>(...)

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm64.cc:5504: return;
any reason for implicit return?

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1529: ASSERT(cids_[i].cid_end ==
kSmiCid);
it is not completely clear to me why we can assume that the only range that
starts with smi is also ending with smi?

maybe add a comment why this assertion holds

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

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:5848: return;
delete return;

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:5088: return;
remove return (I guess all EmitNullCheck have this)

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:5829: return;
ditto

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:537: value_check->Add(new (zone)
CidRange(value_check_cid, value_check_cid));
can this be Cids::CreateMonomorphic?

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:739: value_check->Add(new (zone)
CidRange(kFloat32x4Cid, kFloat32x4Cid));
can this be Cids::CreateMonomorphic()?

[erikcorry, 2017-05-10 08:47:44]

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:2112: const Cids* cids = Cids::Create(Z, ic_data,
0);
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> please add a comment here about what does 0 means.

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/aot_optimizer.h
File runtime/vm/aot_optimizer.h (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.h:96: void AddChecksToArgNr(InstanceCallInstr* call,
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> s/ToArgNr/ForArgNr/ ?

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/assembler_ar...
File runtime/vm/assembler_arm64.h (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/assembler_ar...
runtime/vm/assembler_arm64.h:1172: void AddImmediate(Register dest, int64_t
imm);
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> It does not seem like anybody is actually using this function.

Gone

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

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:171: bool Cids::ContainsExternalizableCids()
const {
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> How about moving Cids methods all together instead of interspersing them with
> methods of other classes?

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:280: if (1ul + cid_end - cid_start >=
sizeof(intptr_t) * 8) {
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> there is kBitsPerWord
> 
> make a variable for cid_end - cid_start + 1 and use it here and below.

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2769: const TargetInfo* target_info_a =
reinterpret_cast<const TargetInfo*>(*a);
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> static_cast instead.

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:2902: if (cid_ranges_[dest]->cid_end + 1 >=
cid_ranges_[src]->cid_start &&
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> for uniformity use TargetAt(x) instead of cid_ranges_[x] consistently in this
> function.

Done, but the assignment in the else clause needs to use cid_ranges_... to work.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:558: CidRange(const CidRange& o)
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> Please order constructors before fields.

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:562: CidRange(intptr_t cid_start_arg,
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> I don't think this constructor is used.

Gone

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:650: return
reinterpret_cast<TargetInfo*>(At(i));
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> static_cast<TargetInfo*>(...)

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm64.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm64.cc:5504: return;
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> any reason for implicit return?

Gone

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1529: ASSERT(cids_[i].cid_end ==
kSmiCid);
On 2017/05/09 21:07:27, Vyacheslav Egorov (Google) wrote:
> it is not completely clear to me why we can assume that the only range that
> starts with smi is also ending with smi?
> 
> maybe add a comment why this assertion holds

Comment added:  // We are in the else clause.

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

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:5848: return;
On 2017/05/09 21:07:28, Vyacheslav Egorov (Google) wrote:
> delete return;

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:5088: return;
On 2017/05/09 21:07:28, Vyacheslav Egorov (Google) wrote:
> remove return (I guess all EmitNullCheck have this)

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:5829: return;
On 2017/05/09 21:07:28, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:537: value_check->Add(new (zone)
CidRange(value_check_cid, value_check_cid));
On 2017/05/09 21:07:28, Vyacheslav Egorov (Google) wrote:
> can this be Cids::CreateMonomorphic?

Done.

https://codereview.chromium.org/2856543002/diff/60001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:739: value_check->Add(new (zone)
CidRange(kFloat32x4Cid, kFloat32x4Cid));
On 2017/05/09 21:07:28, Vyacheslav Egorov (Google) wrote:
> can this be Cids::CreateMonomorphic()?

Done.

[erikcorry, 2017-05-11 08:01:11]

Description was changed from

==========
Use off-heap data for class check instructions

This continues the work from https://codereview.chromium.org/2842753002/
- see the motivation there.

R=vegorov@google.com
BUG=
==========

to

==========
Use off-heap data for class check instructions

This continues the work from https://codereview.chromium.org/2842753002/
- see the motivation there.

R=vegorov@google.com
BUG=

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

[erikcorry, 2017-05-11 08:01:13]

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