Enable optimizer pipeline for DBC.

Enable optimizer pipeline for DBC.

Most of the infrastructure is fixed to work with DBC stack layout:

- register allocator allocates DBC registers with the limitation that we allocate only 20 registers and bail out if anything needs spilling (there is no use implementing spilling on DBC because registers are memory locations themselves). We should be able to bump number of CPU registers on DBC up to 256 but this requires major surgery in some parts - so I postponed this;
- lazy deoptimization is implemented, eager deoptimization is not - because we don't emit any code that actually requires it. it's a minor change to support it once we have a target;
- stack scanning respects stack maps built by registers allocator;

We bailout from all unsupported instructions.

R=zra@google.com

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

[Vyacheslav Egorov (Google), 2016-05-18 20:51:14]

Description was changed from

==========
Enable optimizer pipeline for DBC.
==========

to

==========
Enable optimizer pipeline for DBC.

Most of the infrastructure is fixed to work with DBC stack layout:

- register allocator allocates DBC registers with the limitation that we
allocate only 20 registers and bail out if anything needs spilling (there is no
use implementing spilling on DBC because registers are memory locations
themselves). We should be able to bump number of CPU registers on DBC up to 256
but this requires major surgery in some parts - so I postponed this;
- lazy deoptimization is implemented, eager deoptimization is not - because we
don't emit any code that actually requires it. it's a minor change to support it
once we have a target;
- stack scanning respects stack maps built by registers allocator;

We bailout from all unsupported instructions.
==========

[Vyacheslav Egorov (Google), 2016-05-18 20:51:14]

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

[Vyacheslav Egorov (Google), 2016-05-18 20:51:27]

PTAL

[Florian Schneider, 2016-05-19 13:21:38]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2016-05-19 13:21:40]

First round of comments

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1701: #else
Needed?

I think the code above works for DBC as well.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1787: intptr_t* start =
reinterpret_cast<intptr_t*>(caller_frame->fp() -
Maybe a helper FrameStart(frame)?

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:350: //    The meaning of operand A matches that of
an A operand in call
Do you mean the ArgC operand?

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:75: + ((frame->sp() - frame->fp()) / kWordSize)
 // For frame size incl. sp.
In generate it would be nice to encapsulate more of this logic in the class
StackFrame - e.g. in stack_frame.h/.cc Can we make a helper

Here a helper FrameSize(frame)?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:83: #else
A helper FrameStart(frame)?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:91: UNREACHABLE();
UNIMPLEMENTED?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:104: UNREACHABLE();
UNIMPLEMENTED?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:181: if (locs->input_count() !=
current->InputCount()) {
Left-over from debugging?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:614:
flow_graph_.parsed_function().Bailout("FlowGraphAllocator", "Catch");
Add TODO to implement try-catch.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:1950: // We don't support spilling on DBC for
now because we have access
unterminated sentence?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:1951:
flow_graph_.parsed_function().Bailout("FlowGraphAllocator", "SPILL");
Indentation.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:2494: last_used_register_ =
Utils::Maximum(last_used_register_, reg);
Is this reachable?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:3071:
entry->set_spill_slot_count(Utils::Maximum((last_used_cpu_register + 1) +
spill_slot_count is a confusing name... maybe stack_slot_count?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:240: __ Drop1();
__ Drop(2)?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:422: UNREACHABLE();
Use ASSERT instead of if-statement?

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:8199: void DropArguments(intptr_t argc) {
Maybe put only a declaration here, and put the definition into
intermediate_language_dbc.cc.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1414: // Check if single stepping.
Optimized code can't be single stepped.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1439: if (thread->isolate()->single_step()) {
Optimized code can't be single-stepped.

[Vyacheslav Egorov (Google), 2016-05-19 15:19:41]

PTAL

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1701: #else
On 2016/05/19 13:21:39, Florian Schneider wrote:
> Needed?
> 
> I think the code above works for DBC as well.

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1787: intptr_t* start =
reinterpret_cast<intptr_t*>(caller_frame->fp() -
On 2016/05/19 13:21:39, Florian Schneider wrote:
> Maybe a helper FrameStart(frame)?

Done.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:350: //    The meaning of operand A matches that of
an A operand in call
On 2016/05/19 13:21:39, Florian Schneider wrote:
> Do you mean the ArgC operand?

Done.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:75: + ((frame->sp() - frame->fp()) / kWordSize)
 // For frame size incl. sp.
On 2016/05/19 13:21:39, Florian Schneider wrote:
> In generate it would be nice to encapsulate more of this logic in the class
> StackFrame - e.g. in stack_frame.h/.cc Can we make a helper
> 
> Here a helper FrameSize(frame)?

I will keep this code without a helper for now.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:83: #else
On 2016/05/19 13:21:39, Florian Schneider wrote:
> A helper FrameStart(frame)?

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:91: UNREACHABLE();
On 2016/05/19 13:21:39, Florian Schneider wrote:
> UNIMPLEMENTED?

I think UNREACHABLE right now is a better choice - because this code can never
be reached with the current situation.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:104: UNREACHABLE();
On 2016/05/19 13:21:39, Florian Schneider wrote:
> UNIMPLEMENTED?

Ditto.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:181: if (locs->input_count() !=
current->InputCount()) {
On 2016/05/19 13:21:39, Florian Schneider wrote:
> Left-over from debugging?

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:614:
flow_graph_.parsed_function().Bailout("FlowGraphAllocator", "Catch");
On 2016/05/19 13:21:39, Florian Schneider wrote:
> Add TODO to implement try-catch.

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:1950: // We don't support spilling on DBC for
now because we have access
On 2016/05/19 13:21:39, Florian Schneider wrote:
> unterminated sentence?

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:1951:
flow_graph_.parsed_function().Bailout("FlowGraphAllocator", "SPILL");
On 2016/05/19 13:21:40, Florian Schneider wrote:
> Indentation.

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:2494: last_used_register_ =
Utils::Maximum(last_used_register_, reg);
On 2016/05/19 13:21:40, Florian Schneider wrote:
> Is this reachable?

It is - allocating non-free registers does not imply spilling so this code is
reachable.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:3071:
entry->set_spill_slot_count(Utils::Maximum((last_used_cpu_register + 1) +
On 2016/05/19 13:21:40, Florian Schneider wrote:
> spill_slot_count is a confusing name... maybe stack_slot_count?

I added a comment.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:240: __ Drop1();
On 2016/05/19 13:21:40, Florian Schneider wrote:
> __ Drop(2)?

Explained why Drop(1)

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:422: UNREACHABLE();
On 2016/05/19 13:21:40, Florian Schneider wrote:
> Use ASSERT instead of if-statement?

Done.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:8199: void DropArguments(intptr_t argc) {
On 2016/05/19 13:21:40, Florian Schneider wrote:
> Maybe put only a declaration here, and put the definition into
> intermediate_language_dbc.cc.

Done.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1414: // Check if single stepping.
On 2016/05/19 13:21:40, Florian Schneider wrote:
> Optimized code can't be single stepped.

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1439: if (thread->isolate()->single_step()) {
On 2016/05/19 13:21:40, Florian Schneider wrote:
> Optimized code can't be single-stepped.

Done.

[zra, 2016-05-19 16:24:28]

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1643: static const intptr_t
kNumberOfSavedCpuRegisters = kNumberOfFpuRegisters;
kNumberOfCpuRegisters?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_patcher...
File runtime/vm/code_patcher_dbc.cc (left):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_patcher...
runtime/vm/code_patcher_dbc.cc:35: ASSERT(start +
CallPattern::DeoptCallPatternLengthInBytes() <= target);
Is this removed because 'target' can be 0? Maybe amend the assert instead of
removing entirely.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/constants_dbc.h
File runtime/vm/constants_dbc.h (left):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:140: //  - InstanceCall ArgC, D; InstanceCall2 ArgC,
D; InstanceCall3 ArgC, D
Was InstanceCall3 never used?

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:437: V(BooleanNegate,                 A_D, reg, reg,
___) \
Missing from comments.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (left):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:732: dest_addr - (kSavedCallerFpSlotFromFp *
kWordSize)));
Oof. How was this working?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:91: UNREACHABLE();
On 2016/05/19 13:21:39, Florian Schneider wrote:
> UNIMPLEMENTED?

I guess it depends on whether we plan to ever unbox doubles?

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:129: 
Extra newline.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:3273: 
Extra newline.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:464: if (!comparison()->HasLocs())
return NULL;
Please use {}

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:108: #undef MAP_CLASS_ID
#undef DEFINE_CASTS ?

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/stack_frame....
runtime/vm/stack_frame.cc:205: visitor->VisitPointers(first, last);
Same in both.

[Vyacheslav Egorov (Google), 2016-05-20 12:11:47]

PTAL

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1643: static const intptr_t
kNumberOfSavedCpuRegisters = kNumberOfFpuRegisters;
On 2016/05/19 16:24:27, zra wrote:
> kNumberOfCpuRegisters?

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_patcher...
File runtime/vm/code_patcher_dbc.cc (left):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/code_patcher...
runtime/vm/code_patcher_dbc.cc:35: ASSERT(start +
CallPattern::DeoptCallPatternLengthInBytes() <= target);
On 2016/05/19 16:24:27, zra wrote:
> Is this removed because 'target' can be 0? Maybe amend the assert instead of
> removing entirely.

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/constants_dbc.h
File runtime/vm/constants_dbc.h (left):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:140: //  - InstanceCall ArgC, D; InstanceCall2 ArgC,
D; InstanceCall3 ArgC, D
On 2016/05/19 16:24:27, zra wrote:
> Was InstanceCall3 never used?

Yep.

Turns out we removed 3-arg ICs last year. They were used to track stored value
type for []= operator, but IC data was growing without bounds.

Now we only have 1 and 2 checked args versions.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/constants_db...
runtime/vm/constants_dbc.h:437: V(BooleanNegate,                 A_D, reg, reg,
___) \
On 2016/05/19 16:24:27, zra wrote:
> Missing from comments.

Done.

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (left):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:732: dest_addr - (kSavedCallerFpSlotFromFp *
kWordSize)));
On 2016/05/19 16:24:27, zra wrote:
> Oof. How was this working?

kSavedCallerFpSlotFromFp is 0 on all architectures that are not DBC

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:129: 
On 2016/05/19 16:24:27, zra wrote:
> Extra newline.

Done.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:464: if (!comparison()->HasLocs())
return NULL;
On 2016/05/19 16:24:27, zra wrote:
> Please use {}

Done.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:108: #undef MAP_CLASS_ID
On 2016/05/19 16:24:28, zra wrote:
> #undef DEFINE_CASTS ?

Done.

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

https://codereview.chromium.org/1992963002/diff/20001/runtime/vm/stack_frame....
runtime/vm/stack_frame.cc:205: visitor->VisitPointers(first, last);
On 2016/05/19 16:24:28, zra wrote:
> Same in both.

Done.

[zra, 2016-05-20 16:56:05]

lgtm

Is optimization now turned on for DBC?

Do I understand correctly that the Bailouts will cause falling back on
unoptimized code?

What are the next steps?

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

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1774: // TODO(turnidge): Compute the start of the
dest frame in the
rm TODO?

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

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:73: ASSERT(cpu_registers_ != NULL);
Would it make sense to keep any of these asserts?

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:91: UNREACHABLE();
Maybe instead of this #ifdef do
ASSERT(FlowGraphCompiler::SupportsUnboxedDoubles())?

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:104: UNREACHABLE();
ditto with FlowGraphCompiler::SupportsUnboxedSimd128()

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:240: // all other arguments are poped.
popped

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:405: __ Move(destination.reg(),
-kParamEndSlotFromFp + source.stack_index());
This pattern (-kParamEndSlotFromFP + something) appears in a few places. Maybe
add a static inline with a nice name to stack_frame_dbc.h for it.

[Vyacheslav Egorov (Google), 2016-05-24 12:35:52]

Description was changed from

==========
Enable optimizer pipeline for DBC.

Most of the infrastructure is fixed to work with DBC stack layout:

- register allocator allocates DBC registers with the limitation that we
allocate only 20 registers and bail out if anything needs spilling (there is no
use implementing spilling on DBC because registers are memory locations
themselves). We should be able to bump number of CPU registers on DBC up to 256
but this requires major surgery in some parts - so I postponed this;
- lazy deoptimization is implemented, eager deoptimization is not - because we
don't emit any code that actually requires it. it's a minor change to support it
once we have a target;
- stack scanning respects stack maps built by registers allocator;

We bailout from all unsupported instructions.
==========

to

==========
Enable optimizer pipeline for DBC.

Most of the infrastructure is fixed to work with DBC stack layout:

- register allocator allocates DBC registers with the limitation that we
allocate only 20 registers and bail out if anything needs spilling (there is no
use implementing spilling on DBC because registers are memory locations
themselves). We should be able to bump number of CPU registers on DBC up to 256
but this requires major surgery in some parts - so I postponed this;
- lazy deoptimization is implemented, eager deoptimization is not - because we
don't emit any code that actually requires it. it's a minor change to support it
once we have a target;
- stack scanning respects stack maps built by registers allocator;

We bailout from all unsupported instructions.

R=zra@google.com

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

[Vyacheslav Egorov (Google), 2016-05-24 12:35:54]

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

[Vyacheslav Egorov (Google), 2016-05-24 12:42:20]

Yes, this enables most of the fundamental infrastructure needed for optimizer
and enables the optimizer itself - so we will now produce optimized code for the
hot functions.

Next steps are reducing amount of bailouts caused by unsupported instructions
and supporting things like unboxed doubles.

I think next good step is to look into BinarySmi ops and like - currently we
don't have specialized BinarySmi ops. The first substep of that would be fixing
type feedback collection for Binary Smi ops - currently we don't update ICData
if we always succeed on the fast path (AddTOS skips InstanceCallX) so optimizer
does not have access to the proper type feedback. Supporting BinarySmiOps also
requires supporting Eager deoptimization - but that should be very easy as Deopt
instructions is already available and code for eager deopt would be almost the
same as for lazy deopt - you just need to remove all code related to preserving
the return value. 

Feel free to ping me if you have any other question regarding future steps with
DBC. We can even have a 1-1 meeting to sketch the plan.

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

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1774: // TODO(turnidge): Compute the start of the
dest frame in the
On 2016/05/20 16:56:05, zra wrote:
> rm TODO?

Done.

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

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:73: ASSERT(cpu_registers_ != NULL);
On 2016/05/20 16:56:05, zra wrote:
> Would it make sense to keep any of these asserts?

Done.

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:91: UNREACHABLE();
On 2016/05/20 16:56:05, zra wrote:
> Maybe instead of this #ifdef do
> ASSERT(FlowGraphCompiler::SupportsUnboxedDoubles())?

Done.

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:104: UNREACHABLE();
On 2016/05/20 16:56:05, zra wrote:
> ditto with FlowGraphCompiler::SupportsUnboxedSimd128()

Done.

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:240: // all other arguments are poped.
On 2016/05/20 16:56:05, zra wrote:
> popped

Done.

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_dbc.cc:405: __ Move(destination.reg(),
-kParamEndSlotFromFp + source.stack_index());
On 2016/05/20 16:56:05, zra wrote:
> This pattern (-kParamEndSlotFromFP + something) appears in a few places. Maybe
> add a static inline with a nice name to stack_frame_dbc.h for it.

It appears in two places that I could find and they are a bit different. So I
would keep them split for now.

[Florian Schneider, 2016-05-25 11:28:40]

Flushing left-over comment

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

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:116: intptr_t* FrameBase(const StackFrame*
frame) {
static?

[Florian Schneider, 2016-05-25 11:29:46]

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

https://codereview.chromium.org/1992963002/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.h:116: intptr_t* FrameBase(const StackFrame*
frame) {
On 2016/05/25 11:28:40, Florian Schneider wrote:
> static?

Never mind... num_args_ is used.