Initial SIMDBC interpreter.

Dart Byte Code interpreter.

This version is Clang/GCC only and does not support Windows because it uses computed goto's.

Only unoptimized mode is supported.

Architecture is described in constants_dbc.h and stack_frame_dbc.h.

R=fschneider@google.com, zra@google.com

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

[Vyacheslav Egorov (Google), 2016-04-08 14:35:13]

Description was changed from

==========
Initial SIMDBC interpreter.

BUG=
==========

to

==========
Dart Byte Code interpreter. 

This version is Clang/GCC only and does not support Windows because it uses
computed goto's.
==========

[Vyacheslav Egorov (Google), 2016-04-08 14:35:13]

vegorov@google.com changed reviewers:
+ fschneider@google.com, iposva@google.com, zra@google.com

[Vyacheslav Egorov (Google), 2016-04-08 14:35:51]

Description was changed from

==========
Dart Byte Code interpreter. 

This version is Clang/GCC only and does not support Windows because it uses
computed goto's.
==========

to

==========
Dart Byte Code interpreter. 

This version is Clang/GCC only and does not support Windows because it uses
computed goto's.

Only unoptimized mode is supported. 

Architecture is described in constants_dbc.h and stack_frame_dbc.h.
==========

[Vyacheslav Egorov (Google), 2016-04-08 15:20:09]

PTAL

[zra, 2016-04-08 22:37:35]

Initial comments. Will continue next week.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/assembler_db...
File runtime/vm/assembler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/assembler_db...
runtime/vm/assembler_dbc.cc:105: ASSERT(!label->IsBound());
duplicate line.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/assembler_te...
File runtime/vm/assembler_test.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/assembler_te...
runtime/vm/assembler_test.cc:14: #if !defined(TARGET_ARCH_DBC)
Re-enable and add failing tests to the status file?

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_patcher...
File runtime/vm/code_patcher_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_patcher...
runtime/vm/code_patcher_dbc.cc:53: // The instance call instruction sequence has
a variable size on DBC.
Does it?

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/compiler_tes...
File runtime/vm/compiler_test.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/compiler_tes...
runtime/vm/compiler_test.cc:115: #if !defined(TARGET_ARCH_DBC)
enable and mark in status file?

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:30: //   FP - base of the cFurrent frame
cFurrent -> current

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:128: //    Load value at index D from constant pool
into rA or push it onto the
FP[rA]?

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:169: //  - IfNeStrictTOS; IfEqStrictTOS;
IfNeStrictNumTOS; IfEqStrictNumTOS
What does Strict mean here? Will there be non-Strict variants?

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:332: V(Jump,                            T, tgt, ___,
___) \
tgt is not in the key above.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:434: ASSERT(op == Bytecode::kStaticCall ||
parens around each 'a == b'

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/cpu_dbc.cc#n...
runtime/vm/cpu_dbc.cc:8: #include "vm/assembler.h"
prune headers.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/cpuinfo_test...
runtime/vm/cpuinfo_test.cc:12: #if !defined(TARGET_ARCH_DBC)
Can this be enabled?

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/dart_entry.c...
runtime/vm/dart_entry.cc:109: invokestub entrypoint =
reinterpret_cast<invokestub>(
Maybe move into #elif and #else below.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/dart_entry.h...
runtime/vm/dart_entry.h:109: friend class Simulator;
Any reason not to make the added functions public?

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:872: intptr_t reverse_index = num_parameters - index;
Maybe move to else branch.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_api...
File runtime/vm/debugger_api_impl_test.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:501: #if !defined(TARGET_ARCH_DBC)
enable and mark in status file?

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:601: #if !defined(TARGET_ARCH_DBC)
ditto

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:13: #include "vm/log.h"
move above stub_code.h

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:30: saved_value_ =
reinterpret_cast<uint32_t*>(pc_)[-1];
A comment about what is going on here could be useful.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:34: // DebugBreak has A operand matching the call it
replaces.
an operand

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:37: reinterpret_cast<uint32_t*>(pc_)[-1] =
Since this appears a few times, maybe calculate the address above with a comment
about what it is and then dereference later.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:68: reinterpret_cast<uint32_t*>(pc_)[-1] =
saved_value_;
Yah, maybe add something to CodeBreakpoint to calculate this address.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:664: #if !defined(TARGET_ARCH_DBC)
Personal note. Stopping here.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/stack_frame_...
File runtime/vm/stack_frame_dbc.h (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/stack_frame_...
runtime/vm/stack_frame_dbc.h:34: * against a slot indicates that is can be
traversed during GC
that *it* can be

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/stack_frame_...
runtime/vm/stack_frame_dbc.h:54: // this indices during code generate in the
backend.
these ... generation

[Vyacheslav Egorov (Google), 2016-04-11 10:49:11]

Comments addressed. New version uploaded.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/assembler_db...
File runtime/vm/assembler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/assembler_db...
runtime/vm/assembler_dbc.cc:105: ASSERT(!label->IsBound());
On 2016/04/08 22:37:34, zra wrote:
> duplicate line.

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/assembler_te...
File runtime/vm/assembler_test.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/assembler_te...
runtime/vm/assembler_test.cc:14: #if !defined(TARGET_ARCH_DBC)
On 2016/04/08 22:37:34, zra wrote:
> Re-enable and add failing tests to the status file?

This test does not compile, so I can't re-enable it via status file.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_patcher...
File runtime/vm/code_patcher_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_patcher...
runtime/vm/code_patcher_dbc.cc:53: // The instance call instruction sequence has
a variable size on DBC.
On 2016/04/08 22:37:34, zra wrote:
> Does it?

Removed the comment

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/compiler_tes...
File runtime/vm/compiler_test.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/compiler_tes...
runtime/vm/compiler_test.cc:115: #if !defined(TARGET_ARCH_DBC)
On 2016/04/08 22:37:34, zra wrote:
> enable and mark in status file?

Done.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:30: //   FP - base of the cFurrent frame
On 2016/04/08 22:37:34, zra wrote:
> cFurrent -> current

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:128: //    Load value at index D from constant pool
into rA or push it onto the
On 2016/04/08 22:37:34, zra wrote:
> FP[rA]?

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:169: //  - IfNeStrictTOS; IfEqStrictTOS;
IfNeStrictNumTOS; IfEqStrictNumTOS
On 2016/04/08 22:37:34, zra wrote:
> What does Strict mean here? Will there be non-Strict variants?

Strict means the same it means in Token::kEQ_STRICT :)

I don't think there'll be a non strict comparison in the bytecode.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:332: V(Jump,                            T, tgt, ___,
___) \
On 2016/04/08 22:37:34, zra wrote:
> tgt is not in the key above.

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:434: ASSERT(op == Bytecode::kStaticCall ||
On 2016/04/08 22:37:34, zra wrote:
> parens around each 'a == b'

Done.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/cpu_dbc.cc#n...
runtime/vm/cpu_dbc.cc:8: #include "vm/assembler.h"
On 2016/04/08 22:37:34, zra wrote:
> prune headers.

Done.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/cpuinfo_test...
runtime/vm/cpuinfo_test.cc:12: #if !defined(TARGET_ARCH_DBC)
On 2016/04/08 22:37:34, zra wrote:
> Can this be enabled?

It can be, but it's a very meaningless test for SIMDBC.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/dart_entry.c...
runtime/vm/dart_entry.cc:109: invokestub entrypoint =
reinterpret_cast<invokestub>(
On 2016/04/08 22:37:34, zra wrote:
> Maybe move into #elif and #else below.

That would cause it to be duplicated twice. Not sure if it's better.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/dart_entry.h...
runtime/vm/dart_entry.h:109: friend class Simulator;
On 2016/04/08 22:37:34, zra wrote:
> Any reason not to make the added functions public?

Simulator also accesses kPositionalCountIndex which is private. We can make
everything public.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:872: intptr_t reverse_index = num_parameters - index;
On 2016/04/08 22:37:34, zra wrote:
> Maybe move to else branch.

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_api...
File runtime/vm/debugger_api_impl_test.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:501: #if !defined(TARGET_ARCH_DBC)
On 2016/04/08 22:37:35, zra wrote:
> enable and mark in status file?

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_api...
runtime/vm/debugger_api_impl_test.cc:601: #if !defined(TARGET_ARCH_DBC)
On 2016/04/08 22:37:34, zra wrote:
> ditto

Done.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:13: #include "vm/log.h"
On 2016/04/08 22:37:35, zra wrote:
> move above stub_code.h

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:30: saved_value_ =
reinterpret_cast<uint32_t*>(pc_)[-1];
On 2016/04/08 22:37:35, zra wrote:
> A comment about what is going on here could be useful.

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:34: // DebugBreak has A operand matching the call it
replaces.
On 2016/04/08 22:37:35, zra wrote:
> an operand

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:37: reinterpret_cast<uint32_t*>(pc_)[-1] =
On 2016/04/08 22:37:35, zra wrote:
> Since this appears a few times, maybe calculate the address above with a
comment
> about what it is and then dereference later.

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/debugger_dbc...
runtime/vm/debugger_dbc.cc:68: reinterpret_cast<uint32_t*>(pc_)[-1] =
saved_value_;
On 2016/04/08 22:37:35, zra wrote:
> Yah, maybe add something to CodeBreakpoint to calculate this address.

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:664: #if !defined(TARGET_ARCH_DBC)
On 2016/04/08 22:37:35, zra wrote:
> Personal note. Stopping here.

Acknowledged.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/stack_frame_...
File runtime/vm/stack_frame_dbc.h (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/stack_frame_...
runtime/vm/stack_frame_dbc.h:34: * against a slot indicates that is can be
traversed during GC
On 2016/04/08 22:37:35, zra wrote:
> that *it* can be

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/stack_frame_...
runtime/vm/stack_frame_dbc.h:54: // this indices during code generate in the
backend.
On 2016/04/08 22:37:35, zra wrote:
> these ... generation

Done.

[Florian Schneider, 2016-04-12 02:29:59]

First round of comments

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_generat...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1302: uword stack_pos = Simulator::Current()->sp();
Maybe just add a helper sp() to the other simulators.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1449: #endif
Maybe add TODO to add OSR support in the future?  Add

#else
  UNREACHABLE()
#endif  // TARGET_ARCH_DBC

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1524: SP[2] = SP[-1];  // type
Would it make sense to have a fast-path new-space allocation with a fall-back to
the runtime call? maybe add a TODO?

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1687: // THR_Print("{%p} %p[%d] = %p\n", pc, array,
Smi::Value(index), value);
Left over debug printing

https://codereview.chromium.org/1858283002/diff/100001/runtime/tests/vm/vm.st...
File runtime/tests/vm/vm.status (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/tests/vm/vm.st...
runtime/tests/vm/vm.status:151: # TODO(vegorov) Optimizing compiler is disable
for the SIMDBC
s/disable/disabled/

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/constants_d...
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/constants_d...
runtime/vm/constants_dbc.h:119: //    Note: rX is singed so it can be used to
address parameters which are
s/singed/signed

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/constants_d...
runtime/vm/constants_dbc.h:243: //    2 * i + 1 default value. rA part of the
LoadConsant bytecode specifies
s/LoadConsant/LoadConstant/

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/constants_d...
runtime/vm/constants_dbc.h:415: static Instr Encode(Opcode op, uint32_t abc) {
Maybe also add an assert that abc fits into range:
ASSERT((abc << 8) >> 8 == abc);

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/disassemble...
File runtime/vm/disassembler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/disassemble...
runtime/vm/disassembler_dbc.cc:74: const char* suffix) {
Formatting: I think we tend to do either all in one line, or one argument per
line, if they don't fit on one line.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/disassemble...
runtime/vm/disassembler_dbc.cc:155: static BytecodeFormatter kFormatters[] = {
Add const?

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/object_dbc_...
File runtime/vm/object_dbc_test.cc (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/object_dbc_...
runtime/vm/object_dbc_test.cc:14: 
Is this file needed?

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:678: static void* dispatch[] = {
Add const?

const void* const

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1686: // THR_Print("{%p} %p[%d] = %p\n", pc, array,
Smi::Value(index), value);
Left over debug printing.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.h (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.h:78: enum InstrinsicId {
s/InstrinsicId/IntrinsicId/g

[Vyacheslav Egorov (Google), 2016-04-12 09:01:22]

PTAL

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_generat...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1302: uword stack_pos = Simulator::Current()->sp();
On 2016/04/12 02:29:58, Florian Schneider wrote:
> Maybe just add a helper sp() to the other simulators.

Done.

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1449: #endif
On 2016/04/12 02:29:58, Florian Schneider wrote:
> Maybe add TODO to add OSR support in the future?  Add
> 
> #else
>   UNREACHABLE()
> #endif  // TARGET_ARCH_DBC

Done.

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

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1524: SP[2] = SP[-1];  // type
On 2016/04/12 02:29:58, Florian Schneider wrote:
> Would it make sense to have a fast-path new-space allocation with a fall-back
to
> the runtime call? maybe add a TODO?

Yes, I think it does makes sense to have a fast-path for new space allocation.
Though it needs to be measured. Added TODO

https://codereview.chromium.org/1858283002/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1687: // THR_Print("{%p} %p[%d] = %p\n", pc, array,
Smi::Value(index), value);
On 2016/04/12 02:29:59, Florian Schneider wrote:
> Left over debug printing

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/tests/vm/vm.st...
File runtime/tests/vm/vm.status (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/tests/vm/vm.st...
runtime/tests/vm/vm.status:151: # TODO(vegorov) Optimizing compiler is disable
for the SIMDBC
On 2016/04/12 02:29:59, Florian Schneider wrote:
> s/disable/disabled/

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/constants_d...
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/constants_d...
runtime/vm/constants_dbc.h:119: //    Note: rX is singed so it can be used to
address parameters which are
On 2016/04/12 02:29:59, Florian Schneider wrote:
> s/singed/signed

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/constants_d...
runtime/vm/constants_dbc.h:243: //    2 * i + 1 default value. rA part of the
LoadConsant bytecode specifies
On 2016/04/12 02:29:59, Florian Schneider wrote:
> s/LoadConsant/LoadConstant/

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/constants_d...
runtime/vm/constants_dbc.h:415: static Instr Encode(Opcode op, uint32_t abc) {
On 2016/04/12 02:29:59, Florian Schneider wrote:
> Maybe also add an assert that abc fits into range:
> ASSERT((abc << 8) >> 8 == abc);

Removed this function - it was incorrectly used to encode A variants.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/disassemble...
File runtime/vm/disassembler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/disassemble...
runtime/vm/disassembler_dbc.cc:74: const char* suffix) {
On 2016/04/12 02:29:59, Florian Schneider wrote:
> Formatting: I think we tend to do either all in one line, or one argument per
> line, if they don't fit on one line.

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/disassemble...
runtime/vm/disassembler_dbc.cc:155: static BytecodeFormatter kFormatters[] = {
On 2016/04/12 02:29:59, Florian Schneider wrote:
> Add const?

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/object_dbc_...
File runtime/vm/object_dbc_test.cc (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/object_dbc_...
runtime/vm/object_dbc_test.cc:14: 
On 2016/04/12 02:29:59, Florian Schneider wrote:
> Is this file needed?

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:678: static void* dispatch[] = {
On 2016/04/12 02:29:59, Florian Schneider wrote:
> Add const?
> 
> const void* const

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1686: // THR_Print("{%p} %p[%d] = %p\n", pc, array,
Smi::Value(index), value);
On 2016/04/12 02:29:59, Florian Schneider wrote:
> Left over debug printing.

Done.

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.h (right):

https://codereview.chromium.org/1858283002/diff/100001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.h:78: enum InstrinsicId {
On 2016/04/12 02:29:59, Florian Schneider wrote:
> s/InstrinsicId/IntrinsicId/g

Done.

[Vyacheslav Egorov (Google), 2016-04-12 17:38:42]

Added comments next to various ifdef's

[Florian Schneider, 2016-04-12 18:01:43]

Lgtm.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:212: }
delete[] opt_param;
delete[] opt_param_position;

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:307: typedef int32_t
(*SimulatorLeafRuntimeCall)(int32_t r0, int32_t r1, int32_t r2,
Formatting: one arg per line. Here and the places below in this file.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:438: DART_FORCE_INLINE static int32_t
SMI_DIV(int32_t lhs, int32_t rhs,
I don't see uses of SMI_DIV? Is the fast-path code missing? i.e. DivTOS
bytecode. Maybe add TODO?

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1288: INVOKE_NATIVE(native_target, args,
FALLTHROUGH) else { SP -= 1; }
Formatting: newline after {

[Vyacheslav Egorov (Google), 2016-04-14 13:05:26]

Thanks for the review, Florian! Comments addressed.

Waiting on Zach for more comments.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:212: }
On 2016/04/12 18:01:42, Florian Schneider wrote:
> delete[] opt_param;
> delete[] opt_param_position;

Switched to Zone allocation.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:307: typedef int32_t
(*SimulatorLeafRuntimeCall)(int32_t r0, int32_t r1, int32_t r2,
On 2016/04/12 18:01:42, Florian Schneider wrote:
> Formatting: one arg per line. Here and the places below in this file.

Done.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:438: DART_FORCE_INLINE static int32_t
SMI_DIV(int32_t lhs, int32_t rhs,
On 2016/04/12 18:01:42, Florian Schneider wrote:
> I don't see uses of SMI_DIV? Is the fast-path code missing? i.e. DivTOS
> bytecode. Maybe add TODO?

Deleted.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1288: INVOKE_NATIVE(native_target, args,
FALLTHROUGH) else { SP -= 1; }
On 2016/04/12 18:01:42, Florian Schneider wrote:
> Formatting: newline after {

Done.

[Vyacheslav Egorov (Google), 2016-04-14 13:05:27]

Thanks for the review, Florian! Comments addressed.

Waiting on Zach for more comments.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:212: }
On 2016/04/12 18:01:42, Florian Schneider wrote:
> delete[] opt_param;
> delete[] opt_param_position;

Switched to Zone allocation.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:307: typedef int32_t
(*SimulatorLeafRuntimeCall)(int32_t r0, int32_t r1, int32_t r2,
On 2016/04/12 18:01:42, Florian Schneider wrote:
> Formatting: one arg per line. Here and the places below in this file.

Done.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:438: DART_FORCE_INLINE static int32_t
SMI_DIV(int32_t lhs, int32_t rhs,
On 2016/04/12 18:01:42, Florian Schneider wrote:
> I don't see uses of SMI_DIV? Is the fast-path code missing? i.e. DivTOS
> bytecode. Maybe add TODO?

Deleted.

https://codereview.chromium.org/1858283002/diff/120001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1288: INVOKE_NATIVE(native_target, args,
FALLTHROUGH) else { SP -= 1; }
On 2016/04/12 18:01:42, Florian Schneider wrote:
> Formatting: newline after {

Done.

[zra, 2016-04-14 18:27:50]

There are lots if #ifdefs. I'd recommend using comments to distinguish between
the following cases:

1. TODO(): Not yet implemented. Needs X.
2. TODO(): Doesn't compile because X. Remove after Y is done.
3. Not supported by DBC, and won't ever be supported.

It might also be nice to consistently pick either
#if defined(TARGET_ARCH_DBC)
#else
#endif

or

#if !defined(TARGET_ARCH_DBC)
#else
#endif

Since, AFAIU, code size is not a concern for this target, we can move away from
#ifdefs for the things that won't ever be supported, and prefer something like
if (CPU::IsDBC()) {} or something like that.

Finally, while I'm sure Simulator::Call() is very fast, I wonder if we shouldn't
consider a trade-off there for readability/maintainability.

https://codereview.chromium.org/1858283002/diff/140001/runtime/vm/disassemble...
File runtime/vm/disassembler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/140001/runtime/vm/disassemble...
runtime/vm/disassembler_dbc.cc:17: BYTECODES_LIST(BYTECODE_NAME)
It looks like vm/constants_dbc.h is coming in through vm/disassembler.h, but I'd
prefer explicitly #including it to make it a bit more obvious where
BYTECODES_LIST is coming from.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/constants_d...
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/constants_d...
runtime/vm/constants_dbc.h:330: #define BYTECODES_LIST(V)                       
      \
Maybe DBC_BYTECODES_LIST.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/disassemble...
File runtime/vm/disassembler_test.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/disassemble...
runtime/vm/disassembler_test.cc:12: #if !defined(PRODUCT) &&
!defined(TARGET_ARCH_DBC)
Sorry to keep bothering you about these. Just want to make sure they're all
supposed to be here. This one in particular is a little surprising since it
looks like the disassembler is implemented.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.cc:1291: #endif
add  // !defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.cc:1338: #if !defined(TARGET_ARCH_DBC)
Is this temporary or permanent? Not clear from the comment.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.cc:1428: #endif
add // !defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.cc:1956: #endif
add  // defined(DEBUG) && !defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:376: // DBC emits calls very differently from
all other architectures due to its
I'd leave these in and put UNREACHABLE() or similar in the implementation. The
failure mode here if one of these gets called is a compiler error, which
probably won't actually be easy to interpret. A runtime error could give more
information, and then you can also get a stack trace to see how the bad call is
reached.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:490: #endif
add  // !defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:496: 
Extra space.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:643: // DBC handles type tests differently from
all other architectures due
ditto.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:699: #endif
add  // !defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:732: #if defined(DEBUG) &&
!defined(TARGET_ARCH_DBC)
ditto or add comment.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:35: #define __ assembler->
not needed?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:104: #define __ assembler->
ditto

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:172: const bool has_optional_params =
num_opt_pos_params != 0 ||
parens around clauses.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:199: if (result > 0) break;
{} please

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language.cc:2971: if (InputCount() != 0 && HasTemp()) {
(InputCount() != 0)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language.cc:3113: // instruction otherwise it fall
through.
falls

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

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:129: for (intptr_t i = 0; i <
num_inputs; i++)
{} please

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:343: ASSERT(kind() == Token::kEQ_STRICT
|| kind() == Token::kNE_STRICT);
Add parents and format as above?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:352: ASSERT(kind() == Token::kEQ_STRICT
|| kind() == Token::kNE_STRICT);
ditto

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:552: void
GraphEntryInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
Maybe comment about why this is empty.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:663: if (idx == 0)  return
kNoRepresentation;  // Flexible input representation.
{} please

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:664: if (idx == 1) return kTagged;  //
Index is a smi.
ditto

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intrinsifie...
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intrinsifie...
runtime/vm/intrinsifier.cc:183: #endif
add // defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intrinsifie...
runtime/vm/intrinsifier.cc:904: #endif
add // !defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1516: #if !defined(TARGET_ARCH_DBC)
Maybe instead of #if defined,  add a function to stack_frame_*.h

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1569: #if !defined(TARGET_ARCH_DBC)
ditto

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/locations.h...
runtime/vm/locations.h:344: #if !defined(TARGET_ARCH_DBC)
Why?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/native_argu...
File runtime/vm/native_arguments.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/native_argu...
runtime/vm/native_arguments.h:257: RawObject** argv_;  // Pointer to an array of
arguments to runtime call.
RawObject*** ?

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

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/object.cc#n...
runtime/vm/object.cc:13488: UNIMPLEMENTED();
Will this be implemented, or should this be UNREACHABLE()?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/object.cc#n...
runtime/vm/object.cc:14648: other.IsDartClosureType()) {
Is this a pre-existing bug? Should this fix go in its own CL with a test case?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/object_test...
runtime/vm/object_test.cc:2969: #endif
add  // !defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/runtime_ent...
File runtime/vm/runtime_entry_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/runtime_ent...
runtime/vm/runtime_entry_dbc.cc:16: #define __ assembler->
unused

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:15: #include "vm/simulator.h"
You can probably safely add "vm/simulator_dbc.h" here to silence a linter error
if there is one.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:65: SimulatorSetjmpBuffer* link() { return link_; }
const?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:70: jmp_buf buffer_;
This is private on other architectures. There, the class has a public method
Setjmp that forwards to setjmp(). Why not do the same thing here?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:78: friend class Simulator;
DISALLOW_ALLOCATION()? DISALLOW_COPY_AND_ASSIGN()?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:165: if (!index->IsHeapObject() &&
reinterpret_cast<intptr_t>(index) >= 0 &&
parens around comparisons.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:180: if (!index->IsHeapObject() &&
reinterpret_cast<intptr_t>(index) >= 0 &&
ditto

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:200: if (!index->IsHeapObject() &&
reinterpret_cast<intptr_t>(index) >= 0 &&
ditto. A small cleanup might be to pull this check out into a function. It's a
bit messy and repeated 3 or 4 times.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:217: if (!index->IsHeapObject() &&
reinterpret_cast<intptr_t>(index) >= 0 &&
ditto

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:357: asm volatile(
How do you plan to ensure that the inline assembly here and below has test
coverage?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:450: #define LIKELY(cond) __builtin_expect((cond),
1)
How big is the performance gain from this hint?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:531: CallRuntime(thread, B, top + checked_args + 1 +
1, pc, checked_args + 1,
It might be helpful to add some comments explaining these calculations.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:555: for (intptr_t i = 0, length =
Smi::Value(cache->length_);
This might be easier to read if length is initialized before the for loop.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:593: for (intptr_t i = 0, length =
Smi::Value(cache->length_);
ditto

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:595: if (cache->data()[i + 0] == receiver_cid &&
parens around equality tests

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:633: for (intptr_t i = 0, length =
Smi::Value(cache->length_);
ditto.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:635: if (cache->data()[i + 0] == receiver_cid &&
parens around equality tests and maybe one per line.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:652: static __attribute__((noinline)) bool
InvokeRuntime(
Does inlining these break setjmp/longjmp? If so, it seems like we have problems
elsewhere as well.

It looks like we've been putting function attributes in platform/globals.h. e.g.
DART_NOINLINE. MSVC should respect __declspec(noinline).

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:704: RawObject* Simulator::Call(const Code& code,
Slava, I can't read this code. I'm don't want to stop this from going in, but
this needs to be rewritten so that a normal human can read it.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:887: goto ClosureNoSuchMethodDispatch);
No.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stack_frame.cc
File runtime/vm/stack_frame.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stack_frame...
runtime/vm/stack_frame.cc:174: #endif
add  // !defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stub_code.h
File runtime/vm/stub_code.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stub_code.h...
runtime/vm/stub_code.h:71: #endif
add // defined(TARGET_ARCH_DBC)

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stub_code_d...
File runtime/vm/stub_code_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stub_code_d...
runtime/vm/stub_code_dbc.cc:39: __ Trap();
So, this will fail at runtime. Should it fail at code generation time with
UNREACHABLE, or UNIMPLEMENTED?

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/unit_test.h...
runtime/vm/unit_test.h:379: #if defined(USING_SIMULATOR) &&
!defined(TARGET_ARCH_DBC)
add !defined(TARGET_ARCH_DBC) at endif

https://codereview.chromium.org/1858283002/diff/160001/tools/build.py
File tools/build.py (right):

https://codereview.chromium.org/1858283002/diff/160001/tools/build.py#newcode142
tools/build.py:142: if arch == 'arm' or arch == 'simdbc':
Is this used? There are no simdbc configurations in configurations_android.gypi

[Florian Schneider, 2016-04-15 01:17:28]

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:376: // DBC emits calls very differently from
all other architectures due to its
On 2016/04/14 18:27:48, zra wrote:
> I'd leave these in and put UNREACHABLE() or similar in the implementation. The
> failure mode here if one of these gets called is a compiler error, which
> probably won't actually be easy to interpret. A runtime error could give more
> information, and then you can also get a stack trace to see how the bad call
is
> reached.

I disagree. If we want to avoid the #ifdef here, one alternative could maybe be
to leave declarations in, but omit definitions, so that a use will result in a
linker error which would be fine as well.

[zra, 2016-04-15 14:16:18]

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:887: goto ClosureNoSuchMethodDispatch);
On 2016/04/14 18:27:49, zra wrote:
> No.

I realized I meant to remove this but thought I'd elaborate a bit. It was hard
to find this. If you don't see this it looks like the while loop dives right
into InvokeClosureNoSuchMethod. Passing goto statements to macros is difficult
to notice and decipher. It would be good at least to have more comments
scattered around this code, and probably also a guide somewhere in the simulator
that describes the overall strategy.

[Vyacheslav Egorov (Google), 2016-04-18 15:56:44]

Please take another look

https://codereview.chromium.org/1858283002/diff/140001/runtime/vm/disassemble...
File runtime/vm/disassembler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/140001/runtime/vm/disassemble...
runtime/vm/disassembler_dbc.cc:17: BYTECODES_LIST(BYTECODE_NAME)
On 2016/04/14 18:27:47, zra wrote:
> It looks like vm/constants_dbc.h is coming in through vm/disassembler.h, but
I'd
> prefer explicitly #including it to make it a bit more obvious where
> BYTECODES_LIST is coming from.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/constants_d...
File runtime/vm/constants_dbc.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/constants_d...
runtime/vm/constants_dbc.h:330: #define BYTECODES_LIST(V)                       
      \
On 2016/04/14 18:27:47, zra wrote:
> Maybe DBC_BYTECODES_LIST.

I think it's obvious that it's DBC bytecodes.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/disassemble...
File runtime/vm/disassembler_test.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/disassemble...
runtime/vm/disassembler_test.cc:12: #if !defined(PRODUCT) &&
!defined(TARGET_ARCH_DBC)
On 2016/04/14 18:27:47, zra wrote:
> Sorry to keep bothering you about these. Just want to make sure they're all
> supposed to be here. This one in particular is a little surprising since it
> looks like the disassembler is implemented.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.cc:1291: #endif
On 2016/04/14 18:27:47, zra wrote:
> add  // !defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.cc:1338: #if !defined(TARGET_ARCH_DBC)
On 2016/04/14 18:27:48, zra wrote:
> Is this temporary or permanent? Not clear from the comment.

It's not a TODO - which means it is permanent.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.cc:1428: #endif
On 2016/04/14 18:27:47, zra wrote:
> add // !defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:376: // DBC emits calls very differently from
all other architectures due to its
I am with Florian on this one. I think making them UNREACHABLE is worse because
it leaves dead code behind. I want to explicitly know which parts of pipeline
are used and which ones are not.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:490: #endif
On 2016/04/14 18:27:48, zra wrote:
> add  // !defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:496: 
On 2016/04/14 18:27:48, zra wrote:
> Extra space.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:643: // DBC handles type tests differently from
all other architectures due
On 2016/04/14 18:27:48, zra wrote:
> ditto.

Acknowledged.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:699: #endif
On 2016/04/14 18:27:48, zra wrote:
> add  // !defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler.h:732: #if defined(DEBUG) &&
!defined(TARGET_ARCH_DBC)
On 2016/04/14 18:27:48, zra wrote:
> ditto or add comment.

There is a comment in the CC code. Duplicated it here.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:35: #define __ assembler->
On 2016/04/14 18:27:48, zra wrote:
> not needed?

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:104: #define __ assembler->
On 2016/04/14 18:27:48, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:172: const bool has_optional_params =
num_opt_pos_params != 0 ||
On 2016/04/14 18:27:48, zra wrote:
> parens around clauses.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_compiler_dbc.cc:199: if (result > 0) break;
On 2016/04/14 18:27:48, zra wrote:
> {} please

This code exists at least in 5 copies in the code base. None of them have {}
around break.

I think in a separate CL this code can actually be moved to a shared part of the
flow_graph_compiler

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language.cc:2971: if (InputCount() != 0 && HasTemp()) {
On 2016/04/14 18:27:48, zra wrote:
> (InputCount() != 0)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language.cc:3113: // instruction otherwise it fall
through.
On 2016/04/14 18:27:48, zra wrote:
> falls

Done.

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

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:129: for (intptr_t i = 0; i <
num_inputs; i++)
On 2016/04/14 18:27:48, zra wrote:
> {} please

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:343: ASSERT(kind() == Token::kEQ_STRICT
|| kind() == Token::kNE_STRICT);
On 2016/04/14 18:27:48, zra wrote:
> Add parents and format as above?

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:352: ASSERT(kind() == Token::kEQ_STRICT
|| kind() == Token::kNE_STRICT);
On 2016/04/14 18:27:48, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:552: void
GraphEntryInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
On 2016/04/14 18:27:48, zra wrote:
> Maybe comment about why this is empty.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:663: if (idx == 0)  return
kNoRepresentation;  // Flexible input representation.
On 2016/04/14 18:27:48, zra wrote:
> {} please

This code matches code in all other backends.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intermediat...
runtime/vm/intermediate_language_dbc.cc:664: if (idx == 1) return kTagged;  //
Index is a smi.
On 2016/04/14 18:27:48, zra wrote:
> ditto

This code matches code in all other backends.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intrinsifie...
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intrinsifie...
runtime/vm/intrinsifier.cc:183: #endif
On 2016/04/14 18:27:48, zra wrote:
> add // defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/intrinsifie...
runtime/vm/intrinsifier.cc:904: #endif
On 2016/04/14 18:27:48, zra wrote:
> add // !defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1516: #if !defined(TARGET_ARCH_DBC)
On 2016/04/14 18:27:49, zra wrote:
> Maybe instead of #if defined,  add a function to stack_frame_*.h

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/isolate.cc#...
runtime/vm/isolate.cc:1569: #if !defined(TARGET_ARCH_DBC)
On 2016/04/14 18:27:48, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/locations.h
File runtime/vm/locations.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/locations.h...
runtime/vm/locations.h:344: #if !defined(TARGET_ARCH_DBC)
On 2016/04/14 18:27:49, zra wrote:
> Why?

There was a comment in the CC file explaining. Copied it to here.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/native_argu...
File runtime/vm/native_arguments.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/native_argu...
runtime/vm/native_arguments.h:257: RawObject** argv_;  // Pointer to an array of
arguments to runtime call.
On 2016/04/14 18:27:49, zra wrote:
> RawObject*** ?

It's definitely not a triple indirection. I think original type was bizarre for
no reason. We always pass a pointer to a stack where arguments start. We don't
pass pointer to a pointer.

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

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/object.cc#n...
runtime/vm/object.cc:13488: UNIMPLEMENTED();
On 2016/04/14 18:27:49, zra wrote:
> Will this be implemented, or should this be UNREACHABLE()?

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/object.cc#n...
runtime/vm/object.cc:14648: other.IsDartClosureType()) {
On 2016/04/14 18:27:49, zra wrote:
> Is this a pre-existing bug? Should this fix go in its own CL with a test case?

It's only reproducible/testible on DBC because of how our type-checking is
written. On normal architectures this is handled on fast-path in the assembly
code. 

I discussed with Ivan and he said it's not worth extracting this into a separate
CL.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/object_test...
runtime/vm/object_test.cc:2969: #endif
On 2016/04/14 18:27:49, zra wrote:
> add  // !defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/runtime_ent...
File runtime/vm/runtime_entry_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/runtime_ent...
runtime/vm/runtime_entry_dbc.cc:16: #define __ assembler->
On 2016/04/14 18:27:49, zra wrote:
> unused

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:15: #include "vm/simulator.h"
On 2016/04/14 18:27:49, zra wrote:
> You can probably safely add "vm/simulator_dbc.h" here to silence a linter
error
> if there is one.

No other file does this.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:70: jmp_buf buffer_;
On 2016/04/14 18:27:50, zra wrote:
> This is private on other architectures. There, the class has a public method
> Setjmp that forwards to setjmp(). Why not do the same thing here?

You can't have a method that forwards to setjmp, because "The stack context will
be invalidated if the function which called setjmp() returns.".

The code like this:

bool SimulatorSetjmpBuffer::Setjmp() { return setjmp(&buffer_); } 

if (buffer->Setjmp()) {
  // ...
} else {
  // ...
}

does NOT work. On other architectures Setjmp is present but is actually never
used. Instead we access jmp_buf directly because Simulator is a friend of this
class.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:78: friend class Simulator;
On 2016/04/14 18:27:50, zra wrote:
> DISALLOW_ALLOCATION()? DISALLOW_COPY_AND_ASSIGN()?

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:165: if (!index->IsHeapObject() &&
reinterpret_cast<intptr_t>(index) >= 0 &&
On 2016/04/14 18:27:50, zra wrote:
> parens around comparisons.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:180: if (!index->IsHeapObject() &&
reinterpret_cast<intptr_t>(index) >= 0 &&
On 2016/04/14 18:27:49, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:200: if (!index->IsHeapObject() &&
reinterpret_cast<intptr_t>(index) >= 0 &&
On 2016/04/14 18:27:49, zra wrote:
> ditto. A small cleanup might be to pull this check out into a function. It's a
> bit messy and repeated 3 or 4 times.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:217: if (!index->IsHeapObject() &&
reinterpret_cast<intptr_t>(index) >= 0 &&
On 2016/04/14 18:27:49, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:357: asm volatile(
On 2016/04/14 18:27:49, zra wrote:
> How do you plan to ensure that the inline assembly here and below has test
> coverage?

I welcome suggestions. Ultimately it's not that different from all other
handwritten assembly code. We can move this function to a header and add a test.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:450: #define LIKELY(cond) __builtin_expect((cond),
1)
On 2016/04/14 18:27:49, zra wrote:
> How big is the performance gain from this hint?

Was around 5%-8% when measured originally. Might need re-measurement.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:531: CallRuntime(thread, B, top + checked_args + 1 +
1, pc, checked_args + 1,
On 2016/04/14 18:27:49, zra wrote:
> It might be helpful to add some comments explaining these calculations.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:555: for (intptr_t i = 0, length =
Smi::Value(cache->length_);
On 2016/04/14 18:27:50, zra wrote:
> This might be easier to read if length is initialized before the for loop.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:593: for (intptr_t i = 0, length =
Smi::Value(cache->length_);
On 2016/04/14 18:27:49, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:595: if (cache->data()[i + 0] == receiver_cid &&
On 2016/04/14 18:27:50, zra wrote:
> parens around equality tests

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:633: for (intptr_t i = 0, length =
Smi::Value(cache->length_);
On 2016/04/14 18:27:49, zra wrote:
> ditto.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:635: if (cache->data()[i + 0] == receiver_cid &&
On 2016/04/14 18:27:49, zra wrote:
> parens around equality tests and maybe one per line.

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:652: static __attribute__((noinline)) bool
InvokeRuntime(
On 2016/04/14 18:27:50, zra wrote:
> Does inlining these break setjmp/longjmp? If so, it seems like we have
problems
> elsewhere as well.
> 
> It looks like we've been putting function attributes in platform/globals.h.
e.g.
> DART_NOINLINE. MSVC should respect __declspec(noinline).

Not inlinling these helps to recover a bit of performance on ARM.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:704: RawObject* Simulator::Call(const Code& code,
On 2016/04/14 18:27:49, zra wrote:
> Slava, I can't read this code. I'm don't want to stop this from going in, but
> this needs to be rewritten so that a normal human can read it.

I added more comments. Please take a look again. 

I would certainly like this function to be readable by other people - but you
should approach it with an idea that its a function written in assembly - not
C++.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stack_frame.cc
File runtime/vm/stack_frame.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stack_frame...
runtime/vm/stack_frame.cc:174: #endif
On 2016/04/14 18:27:50, zra wrote:
> add  // !defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stub_code.h
File runtime/vm/stub_code.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stub_code.h...
runtime/vm/stub_code.h:71: #endif
On 2016/04/14 18:27:50, zra wrote:
> add // defined(TARGET_ARCH_DBC)

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stub_code_d...
File runtime/vm/stub_code_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/stub_code_d...
runtime/vm/stub_code_dbc.cc:39: __ Trap();
On 2016/04/14 18:27:50, zra wrote:
> So, this will fail at runtime. Should it fail at code generation time with
> UNREACHABLE, or UNIMPLEMENTED?

Can't make this unreachable because this stub is still generate but not used. 

Needs to be cleaned up. Added TODOs.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/unit_test.h
File runtime/vm/unit_test.h (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/unit_test.h...
runtime/vm/unit_test.h:379: #if defined(USING_SIMULATOR) &&
!defined(TARGET_ARCH_DBC)
On 2016/04/14 18:27:50, zra wrote:
> add !defined(TARGET_ARCH_DBC) at endif

Done.

https://codereview.chromium.org/1858283002/diff/160001/tools/build.py
File tools/build.py (right):

https://codereview.chromium.org/1858283002/diff/160001/tools/build.py#newcode142
tools/build.py:142: if arch == 'arm' or arch == 'simdbc':
On 2016/04/14 18:27:50, zra wrote:
> Is this used? There are no simdbc configurations in
configurations_android.gypi

Yes, I somehow managed to build for Android even though there are none.

[zra, 2016-04-18 16:50:14]

lgtm with nits

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:70: jmp_buf buffer_;
On 2016/04/18 15:56:43, Vyacheslav Egorov (Google) wrote:
> On 2016/04/14 18:27:50, zra wrote:
> > This is private on other architectures. There, the class has a public method
> > Setjmp that forwards to setjmp(). Why not do the same thing here?
> 
> You can't have a method that forwards to setjmp, because "The stack context
will
> be invalidated if the function which called setjmp() returns.".
> 
> The code like this:
> 
> bool SimulatorSetjmpBuffer::Setjmp() { return setjmp(&buffer_); } 
> 
> if (buffer->Setjmp()) {
>   // ...
> } else {
>   // ...
> }
> 
> does NOT work. On other architectures Setjmp is present but is actually never
> used. Instead we access jmp_buf directly because Simulator is a friend of this
> class. 

Please remove or add TODO to remove in the other simulators.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:359: res = __builtin_sadd_overflow(lhs, rhs, out);
It looks like these builtins return bools:

https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:781: if (LIKELY(AreBothSmis(lhs, rhs) && !Func(lhs,
rhs, slot))) {              \
It looks like the functions passed for Func return an int32_t, so this should be
(Func(lhs, rhs, slot) == 0)).

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:787: DISPATCH();                                    
                           \
I'd pull the DISPATCH() out of the macro to match the rest of the code, i.e.:

BYTECODE()
...
DISPATCH()

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:972: if (!(min_num_pos_args <= pos_count &&
pos_count <= max_num_pos_args)) {
parens around comparisons.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:990: while (j >= 0 && i >= 0) {
ditto

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1376: function->ptr()->num_optional_parameters_ == 0
parens

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1396: function->ptr()->num_optional_parameters_ == 0
ditto

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1665: if
(entries[SubtypeTestCache::kInstanceClassIdOrFunction] ==
parens around comparisons

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1700: if (value == true_value || value ==
false_value) {
ditto

[zra, 2016-04-18 16:52:34]

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:704: RawObject* Simulator::Call(const Code& code,
On 2016/04/18 15:56:43, Vyacheslav Egorov (Google) wrote:
> On 2016/04/14 18:27:49, zra wrote:
> > Slava, I can't read this code. I'm don't want to stop this from going in,
but
> > this needs to be rewritten so that a normal human can read it.
> 
> I added more comments. Please take a look again. 
> 
> I would certainly like this function to be readable by other people - but you
> should approach it with an idea that its a function written in assembly - not
> C++.

Much easier now. Thanks!

[Vyacheslav Egorov (Google), 2016-04-18 19:19:57]

Thanks for the review. Landing.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:65: SimulatorSetjmpBuffer* link() { return link_; }
On 2016/04/14 18:27:49, zra wrote:
> const?

Done.

https://codereview.chromium.org/1858283002/diff/160001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:70: jmp_buf buffer_;
On 2016/04/18 16:50:14, zra wrote:
> On 2016/04/18 15:56:43, Vyacheslav Egorov (Google) wrote:
> > On 2016/04/14 18:27:50, zra wrote:
> > > This is private on other architectures. There, the class has a public
method
> > > Setjmp that forwards to setjmp(). Why not do the same thing here?
> > 
> > You can't have a method that forwards to setjmp, because "The stack context
> will
> > be invalidated if the function which called setjmp() returns.".
> > 
> > The code like this:
> > 
> > bool SimulatorSetjmpBuffer::Setjmp() { return setjmp(&buffer_); } 
> > 
> > if (buffer->Setjmp()) {
> >   // ...
> > } else {
> >   // ...
> > }
> > 
> > does NOT work. On other architectures Setjmp is present but is actually
never
> > used. Instead we access jmp_buf directly because Simulator is a friend of
this
> > class. 
> 
> Please remove or add TODO to remove in the other simulators.

Done.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
File runtime/vm/simulator_dbc.cc (right):

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:359: res = __builtin_sadd_overflow(lhs, rhs, out);
On 2016/04/18 16:50:14, zra wrote:
> It looks like these builtins return bools:
> 
> https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

Yes, I am intentionally assigning bool to an int and relying on a standard C++
behavior:  [conv.integral] says "If the source type is bool, the value false is
converted to zero and the value true is converted to one.".

Added cast to make it explicit.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:781: if (LIKELY(AreBothSmis(lhs, rhs) && !Func(lhs,
rhs, slot))) {              \
On 2016/04/18 16:50:14, zra wrote:
> It looks like the functions passed for Func return an int32_t, so this should
be
> (Func(lhs, rhs, slot) == 0)).

Not all of them. SMI_GT for example returns boolean false always. 

I fixed this - made all functions return boolean.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:787: DISPATCH();                                    
                           \
On 2016/04/18 16:50:14, zra wrote:
> I'd pull the DISPATCH() out of the macro to match the rest of the code, i.e.:
> 
> BYTECODE()
> ...
> DISPATCH() 

Done.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:972: if (!(min_num_pos_args <= pos_count &&
pos_count <= max_num_pos_args)) {
On 2016/04/18 16:50:14, zra wrote:
> parens around comparisons.

Done.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:990: while (j >= 0 && i >= 0) {
On 2016/04/18 16:50:14, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1376: function->ptr()->num_optional_parameters_ == 0
On 2016/04/18 16:50:14, zra wrote:
> parens

Done.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1396: function->ptr()->num_optional_parameters_ == 0
On 2016/04/18 16:50:14, zra wrote:
> ditto

Done.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1665: if
(entries[SubtypeTestCache::kInstanceClassIdOrFunction] ==
On 2016/04/18 16:50:14, zra wrote:
> parens around comparisons

Done.

https://codereview.chromium.org/1858283002/diff/180001/runtime/vm/simulator_d...
runtime/vm/simulator_dbc.cc:1700: if (value == true_value || value ==
false_value) {
On 2016/04/18 16:50:14, zra wrote:
> ditto

Done.

[Vyacheslav Egorov (Google), 2016-04-18 21:02:05]

Description was changed from

==========
Dart Byte Code interpreter. 

This version is Clang/GCC only and does not support Windows because it uses
computed goto's.

Only unoptimized mode is supported. 

Architecture is described in constants_dbc.h and stack_frame_dbc.h.
==========

to

==========
Dart Byte Code interpreter.

This version is Clang/GCC only and does not support Windows because it uses
computed goto's.

Only unoptimized mode is supported.

Architecture is described in constants_dbc.h and stack_frame_dbc.h.

R=fschneider@google.com, zra@google.com

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

[Vyacheslav Egorov (Google), 2016-04-18 21:02:07]

Committed patchset #12 (id:220001) manually as
ee0f608ce4d9ba35f360dfec76626ff4613eb103.