VM: Make optimized try-catch work in DBC.

VM: Make optimized try-catch work in DBC.

The catch entry block has all locals in fixed locations
(Rj) where j = kNumberOfRegisters - i for parameter i.

This means we reserve a range of DBC registers at the top-end of the frame.
Those registers are blocked for general allocation to avoid any overlap
with the rest of the registers that are allocated from the bottom.

Each optimized frame with a try-catch will be kNumberOfRegisters wide.

BUG=
R=vegorov@google.com

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

[Florian Schneider, 2016-10-04 00:32:18]

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

[zra, 2016-10-04 15:22:48]

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_alloc...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:634: intptr_t exception_reg =
-catch_entry->exception_var().index() - 1;
Can you use LocalVarIndex() in stack_frame_dbc.h

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:687: if (block->IsGraphEntry() &&
range->End() > kNormalEntryPos) {
Parens around >

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:734: #if !defined(TARGET_ARCH_DBC)
Maybe it would be cleaner to try to use only one #if !defined(TARGET_ARCH_DBC)
here.

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.cc (left):

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:855: bitmap->SetLength(spill_area_size);
Don't you still need this line in non-DBC?

[Florian Schneider, 2016-10-04 17:01:05]

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_alloc...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:634: intptr_t exception_reg =
-catch_entry->exception_var().index() - 1;
On 2016/10/04 15:22:48, zra wrote:
> Can you use LocalVarIndex() in stack_frame_dbc.h

Done. Here and in CatchBlockEntryInstr::EmitNativeCode.

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:687: if (block->IsGraphEntry() &&
range->End() > kNormalEntryPos) {
On 2016/10/04 15:22:48, zra wrote:
> Parens around >

Done.

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:734: #if !defined(TARGET_ARCH_DBC)
On 2016/10/04 15:22:48, zra wrote:
> Maybe it would be cleaner to try to use only one #if !defined(TARGET_ARCH_DBC)
> here.

Done.

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.cc (left):

https://codereview.chromium.org/2388093003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:855: bitmap->SetLength(spill_area_size);
On 2016/10/04 15:22:48, zra wrote:
> Don't you still need this line in non-DBC?

Oops, yes of course. Thanks!

[Vyacheslav Egorov (Google), 2016-10-04 17:07:54]

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

https://codereview.chromium.org/2388093003/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:736: if (defn->IsParameter() &&
(range->spill_slot().stack_index() >= 0)) {
I thought we don't support spilling on DBC. How does this work (e.g. where is
the code that ensures that register numbers for live SSA values and register
numbers for spill space for local variables do not intersect?)

[Florian Schneider, 2016-10-04 17:07:57]

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

https://codereview.chromium.org/2388093003/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:1413: is_dense_switch =
Smi::IsValid(cid_mask);
I'll fix this in a follow-up CL by adding support for adding untagged immediates
to the constant pool. For now, just bail out to the generic CheckClass if mask
is too large for a smi.

[Florian Schneider, 2016-10-04 17:11:02]

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

https://codereview.chromium.org/2388093003/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:736: if (defn->IsParameter() &&
(range->spill_slot().stack_index() >= 0)) {
On 2016/10/04 17:07:54, Vyacheslav Egorov (Google) wrote:
> I thought we don't support spilling on DBC. How does this work (e.g. where is
> the code that ensures that register numbers for live SSA values and register
> numbers for spill space for local variables do not intersect?)

You're right. This should be UNREACHABLE() in DBC...

[Vyacheslav Egorov (Google), 2016-10-04 17:55:31]

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

https://codereview.chromium.org/2388093003/diff/20001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:736: if (defn->IsParameter() &&
(range->spill_slot().stack_index() >= 0)) {
On 2016/10/04 17:11:02, Florian Schneider wrote:
> On 2016/10/04 17:07:54, Vyacheslav Egorov (Google) wrote:
> > I thought we don't support spilling on DBC. How does this work (e.g. where
is
> > the code that ensures that register numbers for live SSA values and register
> > numbers for spill space for local variables do not intersect?)
> 
> You're right. This should be UNREACHABLE() in DBC...
> 

I am still not completely sure how this stuff manages to work on DBC...

I need to patch this in and look at generated bytecode to convince myself :) I
will do it tomorrow morning. Hope this delay is ok.

[Florian Schneider, 2016-10-04 21:51:33]

On 2016/10/04 17:55:31, Vyacheslav Egorov (Google) wrote:
>
https://codereview.chromium.org/2388093003/diff/20001/runtime/vm/flow_graph_a...
> File runtime/vm/flow_graph_allocator.cc (right):
> 
>
https://codereview.chromium.org/2388093003/diff/20001/runtime/vm/flow_graph_a...
> runtime/vm/flow_graph_allocator.cc:736: if (defn->IsParameter() &&
> (range->spill_slot().stack_index() >= 0)) {
> On 2016/10/04 17:11:02, Florian Schneider wrote:
> > On 2016/10/04 17:07:54, Vyacheslav Egorov (Google) wrote:
> > > I thought we don't support spilling on DBC. How does this work (e.g. where
> is
> > > the code that ensures that register numbers for live SSA values and
register
> > > numbers for spill space for local variables do not intersect?)
> > 
> > You're right. This should be UNREACHABLE() in DBC...
> > 
> 
> I am still not completely sure how this stuff manages to work on DBC...
> 
> I need to patch this in and look at generated bytecode to convince myself :) I
> will do it tomorrow morning. Hope this delay is ok.

Let me upload a new patch-set before you take another look: The generation of
the try-sync code emits "Unsupported move" which causes bailout. I need to fix
this, otherwise we don't actually generate optimized code.

[Vyacheslav Egorov (Google), 2016-10-05 05:09:11]

The reason why I don't think the current approach is working is that we need
some code somewhere for blocking first R_0, ..., R_k registers for k locals. and
I can't figure out where this code is :)

Imagine the code like this

v0 <- ...
v1 <- ...
Call() env = {v1}
use(v0)

if v0 is allocated to R0 then we will overwrite it by moving v1 into R0 during
the call.

[Florian Schneider, 2016-10-06 00:41:40]

Description was changed from

==========
VM: Make optimized try-catch work in DBC.

The catch entry block has all locals in fixed locations
(Rj) where j is the slot index for parameter i.

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

to

==========
VM: Make optimized try-catch work in DBC.

The catch entry block has all locals in fixed locations
(Rj) where j = kNumberOfRegisters - i for parameter i.

This means we reserve a range of DBC registers at the top-end of the frame.
Those registers are blocked for general allocation to avoid any overlap
with the rest of the registers that are allocated from the bottom.

Each optimized frame with a try-catch will be kNumberOfRegisters wide.

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

[Florian Schneider, 2016-10-06 00:58:22]

All variables in the catch entry environment get fixed registers assigned from
the top-end of the register range (R63, R62, ... on 64bit DBC).

These registers are marked as blocked for general allocation to avoid overlap
with normal allocation from the bottom.

I'll work on adding a few more tests to improve our test coverage of
try-catch-finally.

Speedup on ParserCombinators: 15%

[Florian Schneider, 2016-10-12 22:21:21]

Ptal

https://codereview.chromium.org/2388093003/diff/60001/tests/corelib/corelib.s...
File tests/corelib/corelib.status (right):

https://codereview.chromium.org/2388093003/diff/60001/tests/corelib/corelib.s...
tests/corelib/corelib.status:211: big_integer_arith_vm_test/modPow: Pass,
RuntimeError # Issue #27474
This is unrelated to try-catch and seems triggered by the fact the different
functions get optimized with my change.

[Vyacheslav Egorov (Google), 2016-10-13 15:02:04]

LGTM

I can't find anything wrong with this approach, though it does make frame of
functions containing try/catch very large with a lot of empty space inside.

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

https://codereview.chromium.org/2388093003/diff/60001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:674:
range->set_assigned_location(Location::RegisterLocation(slot_index));
Some of this code is repeated below just with  kNormalEntryPos instead of
block->lifetime_position() + 2. 

I wonder if it can be refactored into a separate helper method.

https://codereview.chromium.org/2388093003/diff/60001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:694: CompleteRange(tail,
Location::kRegister);
we have UnboxedConstantInstr (which inherits from ConstantInstr), so maybe
assert representation, just in case?

[Florian Schneider, 2016-10-13 17:24:04]

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

https://codereview.chromium.org/2388093003/diff/60001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:674:
range->set_assigned_location(Location::RegisterLocation(slot_index));
On 2016/10/13 15:02:04, Vyacheslav Egorov (Google) wrote:
> Some of this code is repeated below just with  kNormalEntryPos instead of
> block->lifetime_position() + 2. 
> 
> I wonder if it can be refactored into a separate helper method. 
> 

Done.

https://codereview.chromium.org/2388093003/diff/60001/runtime/vm/flow_graph_a...
runtime/vm/flow_graph_allocator.cc:694: CompleteRange(tail,
Location::kRegister);
On 2016/10/13 15:02:04, Vyacheslav Egorov (Google) wrote:
> we have UnboxedConstantInstr (which inherits from ConstantInstr), so maybe
> assert representation, just in case?

Done.

[Florian Schneider, 2016-10-13 17:30:43]

On 2016/10/13 15:02:04, Vyacheslav Egorov (Google) wrote:
> LGTM
> 
> I can't find anything wrong with this approach, though it does make frame of
> functions containing try/catch very large with a lot of empty space inside.
> 

The empty space is not optimal, but I don't think it will be a performance
issue, other than
the DBC stack growing more than for functions without try-catch.

[Florian Schneider, 2016-10-13 18:36:31]

Description was changed from

==========
VM: Make optimized try-catch work in DBC.

The catch entry block has all locals in fixed locations
(Rj) where j = kNumberOfRegisters - i for parameter i.

This means we reserve a range of DBC registers at the top-end of the frame.
Those registers are blocked for general allocation to avoid any overlap
with the rest of the registers that are allocated from the bottom.

Each optimized frame with a try-catch will be kNumberOfRegisters wide.

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

to

==========
VM: Make optimized try-catch work in DBC.

The catch entry block has all locals in fixed locations
(Rj) where j = kNumberOfRegisters - i for parameter i.

This means we reserve a range of DBC registers at the top-end of the frame.
Those registers are blocked for general allocation to avoid any overlap
with the rest of the registers that are allocated from the bottom.

Each optimized frame with a try-catch will be kNumberOfRegisters wide.

BUG=
R=vegorov@google.com

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

[Florian Schneider, 2016-10-13 18:36:32]

Committed patchset #5 (id:80001) manually as
6d66f3dea89027c083af791becbce3dbc6e5d257 (presubmit successful).