DBC: CheckClassInstr

DBC: CheckClassInstr

R=vegorov@google.com

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

[zra, 2016-06-23 23:06:00]

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

[zra, 2016-06-23 23:06:01]

Slava,

There is a problem with materializing objects on deopt, which I've "fixed". The
fields are arranged as if the stack grows the normal way (I think), so I just
changed to iterate in the other direction.

The following tests still fail, and I don't know whether it's on a different bug
or because my "fix" is wrong. If you have time could you please take a look?
Thanks!

Here are the tests that fail:

out/DebugSIMDBC64/dart --optimization-counter-threshold=10 --no-use-osr
--no-background-compilation --ignore-unrecognized-flags
--package-root=out/DebugSIMDBC64/packages/
/usr/local/google/home/zra/dart/sdk/tests/language/vm/allocation_sinking_vm_test.dart

out/DebugSIMDBC64/dart --ignore-unrecognized-flags
--package-root=out/DebugSIMDBC64/packages/
/usr/local/google/home/zra/dart/sdk/tests/co19/src/LibTest/collection/DoubleLinkedQueue/DoubleLinkedQueue_class_A01_t01.dart

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

https://codereview.chromium.org/2098573004/diff/1/runtime/vm/deferred_objects...
runtime/vm/deferred_objects.h:251: #else
Is there a better hack to fix this problem?

[Vyacheslav Egorov (Google), 2016-06-24 14:47:07]

It's my bug in the Deopt instruction. See comment regarding the number of
materialization arguments.

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

https://codereview.chromium.org/2098573004/diff/1/runtime/vm/deferred_objects...
runtime/vm/deferred_objects.h:251: #else
On 2016/06/23 23:06:01, zra wrote:
> Is there a better hack to fix this problem?

How about:

RawObject* GetArg(intptr_t index) {
#if !defined(...)
  return args_[index];
#else
  return args_[-index];
#endif
}

and then using it below instead of args_[...]

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

https://codereview.chromium.org/2098573004/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2231: *++SP = 0;  // Space for the result: number of
materialization args.
the call actually returns number of materialization args * kWordSize

https://codereview.chromium.org/2098573004/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2237: Smi::Value(RAW_CAST(Smi, *SP--));
this needs to be divided by kWordSize.

sorry about that.

[zra, 2016-06-24 22:37:49]

Hi Slava,

Sorry to bother you again with another bug. Your fixes to the Deopt instruction
worked, but the second test above is still failing on the assert I've noted in
the CL. It looks like optimized static calls might not be getting set up
correctly. I tried just making the code look like other platforms, but the
assert is still firing. Any ideas? Thanks!

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

https://codereview.chromium.org/2098573004/diff/1/runtime/vm/deferred_objects...
runtime/vm/deferred_objects.h:251: #else
On 2016/06/24 14:47:07, Vyacheslav Egorov (Google) wrote:
> On 2016/06/23 23:06:01, zra wrote:
> > Is there a better hack to fix this problem?
> 
> How about:
> 
> RawObject* GetArg(intptr_t index) {
> #if !defined(...)
>   return args_[index];
> #else
>   return args_[-index];
> #endif
> }
> 
> and then using it below instead of args_[...]

Done.

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

https://codereview.chromium.org/2098573004/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2231: *++SP = 0;  // Space for the result: number of
materialization args.
On 2016/06/24 14:47:07, Vyacheslav Egorov (Google) wrote:
> the call actually returns number of materialization args * kWordSize

Acknowledged.

https://codereview.chromium.org/2098573004/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2237: Smi::Value(RAW_CAST(Smi, *SP--));
On 2016/06/24 14:47:07, Vyacheslav Egorov (Google) wrote:
> this needs to be divided by kWordSize.
> 
> sorry about that.

Done.

https://codereview.chromium.org/2098573004/diff/20001/runtime/vm/deferred_obj...
File runtime/vm/deferred_objects.cc (right):

https://codereview.chromium.org/2098573004/diff/20001/runtime/vm/deferred_obj...
runtime/vm/deferred_objects.cc:125: ASSERT(code.GetPcForDeoptId(deopt_id_,
RawPcDescriptors::kDeopt) != 0);
Hitting this assert. deopt_id_ is 38. It's listed in the PC Descriptors for the
optimized, but not the unoptimized code. Not sure how to fix.

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

https://codereview.chromium.org/2098573004/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3332: RawPcDescriptors::Kind kind =
(compiler->is_optimizing())
Fixing this doesn't stop the assert from being hit.

[zra, 2016-06-27 16:01:11]

Thanks for explaining how to debug this! I added the fix and the tests now pass.
PTAL.

[Vyacheslav Egorov (Google), 2016-06-28 13:03:11]

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/deferred_obj...
File runtime/vm/deferred_objects.h (right):

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/deferred_obj...
runtime/vm/deferred_objects.h:235: #if !defined(TARGET_ARCH_DBC)
I think we never align #if so it has to look like this:

  RawObject* GetArg(...) {
#if ...
    return

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

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/disassembler...
runtime/vm/disassembler.cc:336: 
two new lines

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/disassembler...
runtime/vm/disassembler.cc:343: 
two new lines

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

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:970: __
PushConstant(Smi::Handle(Smi::New(cids_[0])));
This encoding of the dense switch is suboptimal. I suggest encoding it like
this:

CheckDenseSwitchTOS rA, CanBeSmi
Nop cids[0] 
Nop ComputeCidMask()
Deopt ...

CheckDenseSwitchTOS would then look ahead and decode low and mask from
subsequent NOPs.

If Check succeeds we skip 3 instructions if check fails we skip 2. 

WDYT?

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:974: __ CheckForCid(value, ic_data);
Similar note here. I suggest making this very fast (= no additional memory
loads) for the 1 check. I suggest splitting it into 

CheckCid value, cid

and 

CheckCids value, icdata

for starters.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:977: ICData::kDeoptCheckClass,
Indentation. 

Can this be shared with NullCheck case?

if (IsNullCheck()) {
  ...
} else if (IsDenseSwitch()) {
  ...
} else {
  ...
}
compiler->EmitDeopt(...);

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1982: BYTECODE(CheckNull, A_D);
Consider splitting this instruction into two and naming them:

IfEqNull 

IfNeNull

Then we can use it to emit optimized comparisons for x == null as well.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1996: ICData::Handle(RAW_CAST(ICData,
LOAD_CONSTANT(rD)));
No handle allocation in the interpreter loop! Please use raw pointers.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2000: if (ic_data.GetReceiverClassIdAt(0) ==
kSmiCid) {
this case is unlikely for dense switches. I suggest doing 

if (LIKELY(!is_smi)) {
  ...
} else {
  RawICData* ic_data = RAW_CAST(ICData, LOAD_CONSTANT(rD));
  if (...) {
    ...
  }
}

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2017: const ICData& ic_data =
No Handles.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2028: FlowGraphCompiler::SortICDataByCount(ic_data,
This should all be done in compile time. Sorting it again and again is pure
waste of time.

[zra, 2016-06-28 19:31:40]

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/deferred_obj...
File runtime/vm/deferred_objects.h (right):

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/deferred_obj...
runtime/vm/deferred_objects.h:235: #if !defined(TARGET_ARCH_DBC)
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> I think we never align #if so it has to look like this:
> 
>   RawObject* GetArg(...) {
> #if ...
>     return

Done.

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

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/disassembler...
runtime/vm/disassembler.cc:336: 
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> two new lines

Done.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/disassembler...
runtime/vm/disassembler.cc:343: 
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> two new lines

Done.

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

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:970: __
PushConstant(Smi::Handle(Smi::New(cids_[0])));
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> This encoding of the dense switch is suboptimal. I suggest encoding it like
> this:
> 
> CheckDenseSwitchTOS rA, CanBeSmi
> Nop cids[0] 
> Nop ComputeCidMask()
> Deopt ...
> 
> CheckDenseSwitchTOS would then look ahead and decode low and mask from
> subsequent NOPs.
> 
> If Check succeeds we skip 3 instructions if check fails we skip 2. 
> 
> WDYT?

It looks like cids can be 32-bits, so I think those constants have to go in the
pool, but getting rid of the PushConstants makes sense.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:974: __ CheckForCid(value, ic_data);
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> Similar note here. I suggest making this very fast (= no additional memory
> loads) for the 1 check. I suggest splitting it into 
> 
> CheckCid value, cid
> 
> and 
> 
> CheckCids value, icdata
> 
> for starters.
> 

Done.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:977: ICData::kDeoptCheckClass,
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> Indentation. 
> 
> Can this be shared with NullCheck case?
> 
> if (IsNullCheck()) {
>   ...
> } else if (IsDenseSwitch()) {
>   ...
> } else {
>   ...
> }
> compiler->EmitDeopt(...);

Done.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1982: BYTECODE(CheckNull, A_D);
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> Consider splitting this instruction into two and naming them:
> 
> IfEqNull 
> 
> IfNeNull
> 
> Then we can use it to emit optimized comparisons for x == null as well. 

Done.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1996: ICData::Handle(RAW_CAST(ICData,
LOAD_CONSTANT(rD)));
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> No handle allocation in the interpreter loop! Please use raw pointers.

Done.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2000: if (ic_data.GetReceiverClassIdAt(0) ==
kSmiCid) {
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> this case is unlikely for dense switches. I suggest doing 
> 
> if (LIKELY(!is_smi)) {
>   ...
> } else {
>   RawICData* ic_data = RAW_CAST(ICData, LOAD_CONSTANT(rD));
>   if (...) {
>     ...
>   }
> }

Done.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2017: const ICData& ic_data =
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> No Handles.

Done.

https://codereview.chromium.org/2098573004/diff/40001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2028: FlowGraphCompiler::SortICDataByCount(ic_data,
On 2016/06/28 13:03:10, Vyacheslav Egorov (Google) wrote:
> This should all be done in compile time. Sorting it again and again is pure
> waste of time. 

Done.

[Vyacheslav Egorov (Google), 2016-06-29 17:11:06]

LGTM with suggested changes to the encoding

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

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:374: //  - CheckCid rA, D
32bit build allocates only 16bits for a CID. I think it's fair to assume even in
64-bit build that CID is 16bit (because application with more than 2^16 classes
will not run on 32-bit build anyway).

Also I suggest not having can_be_smi flag for a single cid comparison. Value can
be smi iff cid we are checking against is kSmiCid.

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

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:990: __ Nop(__
AddConstant(sorted_smi_cids));
I suggest just embedding them into the code instead of having an array on the
side. 

I don't think we are saving any memory by doing this. 

Embed CIDs by truncating them to 16bits.

You can have a helper that bails out of optimized compilation if CID is not
16bit.

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:995: ICData::kDeoptCheckClass,
indentation

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2026: if (is_smi) {
Flip this conditions to match CheckDenseSwitch

if (!LIKELY(is_smi)) {
  // ...
} else {
  // ...
}

[Vyacheslav Egorov (Google), 2016-06-29 18:33:10]

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2035: const intptr_t desired_cid =
Smi::Value(RAW_CAST(Smi, cids[i]));
the array of cids is sorted so you can exit the loop early but maybe it does not
matter for small amount of cids.

[zra, 2016-06-29 22:19:20]

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

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:374: //  - CheckCid rA, D
On 2016/06/29 17:11:06, Vyacheslav Egorov (Google) wrote:
> 32bit build allocates only 16bits for a CID. I think it's fair to assume even
in
> 64-bit build that CID is 16bit (because application with more than 2^16
classes
> will not run on 32-bit build anyway).
> 
> Also I suggest not having can_be_smi flag for a single cid comparison. Value
can
> be smi iff cid we are checking against is kSmiCid.
> 
> 

Done.

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

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:990: __ Nop(__
AddConstant(sorted_smi_cids));
On 2016/06/29 17:11:06, Vyacheslav Egorov (Google) wrote:
> I suggest just embedding them into the code instead of having an array on the
> side. 
> 
> I don't think we are saving any memory by doing this. 
> 
> Embed CIDs by truncating them to 16bits.
> 
> You can have a helper that bails out of optimized compilation if CID is not
> 16bit.

Done.

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:995: ICData::kDeoptCheckClass,
On 2016/06/29 17:11:06, Vyacheslav Egorov (Google) wrote:
> indentation

Done.

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/simulator_db...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2026: if (is_smi) {
On 2016/06/29 17:11:06, Vyacheslav Egorov (Google) wrote:
> Flip this conditions to match CheckDenseSwitch
> 
> if (!LIKELY(is_smi)) {
>   // ...
> } else {
>   // ...
> }

Done.

https://codereview.chromium.org/2098573004/diff/80001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2035: const intptr_t desired_cid =
Smi::Value(RAW_CAST(Smi, cids[i]));
On 2016/06/29 18:33:10, Vyacheslav Egorov (Google) wrote:
> the array of cids is sorted so you can exit the loop early but maybe it does
not
> matter for small amount of cids.

Done.

[zra, 2016-06-29 22:25:46]

Description was changed from

==========
DBC: CheckClassInstr
==========

to

==========
DBC: CheckClassInstr

R=vegorov@google.com

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

[zra, 2016-06-29 22:25:48]

Committed patchset #6 (id:100001) manually as
db26281172c734ad77bba837f4b58ac551366dac (presubmit successful).

[Vyacheslav Egorov (Google), 2016-06-30 08:04:13]

https://codereview.chromium.org/2098573004/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2098573004/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:1045: __ CheckClassId(value, cid);
I don't think this is correct if may_be_smi is true - if maybe smi is true then
there are two cids to check.

https://codereview.chromium.org/2098573004/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:1053: if (!Utils::IsUint(16, cid)) {
This should really be a helper, e.g. on compiler

uint16_t cid = compiler->ToEmbeddableCid(intptr_t cid)

[zra, 2016-06-30 15:30:53]

https://codereview.chromium.org/2098573004/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2098573004/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:1045: __ CheckClassId(value, cid);
On 2016/06/30 08:04:13, Vyacheslav Egorov (Google) wrote:
> I don't think this is correct if may_be_smi is true - if maybe smi is true
then
> there are two cids to check. 

Okay. I'm just going to put this back to how it was in the previous
revision---passing may_be_smi and putting the cid in the following Nop.

[Vyacheslav Egorov (Google), 2016-06-30 15:32:09]

https://codereview.chromium.org/2098573004/diff/100001/runtime/vm/intermediat...
File runtime/vm/intermediate_language_dbc.cc (right):

https://codereview.chromium.org/2098573004/diff/100001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:1045: __ CheckClassId(value, cid);
On 2016/06/30 15:30:53, zra wrote:
> On 2016/06/30 08:04:13, Vyacheslav Egorov (Google) wrote:
> > I don't think this is correct if may_be_smi is true - if maybe smi is true
> then
> > there are two cids to check. 
> 
> Okay. I'm just going to put this back to how it was in the previous
> revision---passing may_be_smi and putting the cid in the following Nop.

You can just emit CheckCids instead.