DBC: Eager deoptimization and CheckSmi instruction.

DBC: Eager deoptimization and CheckSmi instruction.

R=vegorov@google.com

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

[zra, 2016-06-07 22:31:28]

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

[zra, 2016-06-07 22:31:29]

I'm having trouble getting this to work. Would you mind taking a look? You can
patch this in and try the test:

out/DebugSIMDBC64/dart --trace_deoptimization -p out/DebugSIMDBC64/packages/
--optimization-counter-threshold=10  --no-background-compilation
tests/language/vm/deopt_smi_check_vm_test.dart

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/deopt_instructio...
File runtime/vm/deopt_instructions.cc (left):

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/deopt_instructio...
runtime/vm/deopt_instructions.cc:75: + ((frame->sp() - frame->fp()) / kWordSize)
 // For frame size incl. sp.
For some reason sp() is < fp() ? Moved around to at least avoid getting
something totally wrong from unsigned arithmetic cast to signed.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_dbc.cc:223: set_pc_offset(assembler->CodeSize());
On other architectures this comes after the Deopt call. Not sure why I have to
put it in front.

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

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:913: __ Push(locs()->in(0).reg());
Thoughts about adding an instruction for this that just checks the tag?

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

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1980: EnterSyntheticFrame(&FP, &SP, pc - 1);
I'm not sure I see how the locals from the optimized frame get shuffled into the
locals for the unoptimized frame...

[Vyacheslav Egorov (Google), 2016-06-08 10:59:10]

I will take a look

[Vyacheslav Egorov (Google), 2016-06-08 11:58:53]

There are two minor off-by-1 in the eager deoptimization code. 

I highlighted both. If they are fixed - deoptimization should start working. 

I tried to explain them.

Please let me know if my explanation helped.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/deopt_instructio...
File runtime/vm/deopt_instructions.cc (left):

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/deopt_instructio...
runtime/vm/deopt_instructions.cc:75: + ((frame->sp() - frame->fp()) / kWordSize)
 // For frame size incl. sp.
On 2016/06/07 22:31:28, zra wrote:
> For some reason sp() is < fp() ? Moved around to at least avoid getting
> something totally wrong from unsigned arithmetic cast to signed.

If SP is less then FP on DBC then this is bug somewhere else.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_dbc.cc:212: // Calls do not need stubs, they
share a deoptimization trampoline.
I don't think we really need DeoptInfoWithStub on DBC. At least not as much as
on other platforms. Instead of doing:

Condition 
Jump stub

We just should do 

Condition 
Deopt ...

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

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:913: __ Push(locs()->in(0).reg());
On 2016/06/07 22:31:28, zra wrote:
> Thoughts about adding an instruction for this that just checks the tag?

Yes, I think this should be just a special bytecode instruction.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:917: __ Jump(deopt);
__ Jump(deopt) could just be __ Deopt(...)

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

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1975: RawObject* result = SP[0];
When not preserving result (non-lazy deopt) we need to preserve SP[0] - it does
not contain result, but contains some temporary expression. So you need to do

if (!preserve_result) SP++; 

before entering the synthetic frame, otherwise SP[0] will be damaged.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1980: EnterSyntheticFrame(&FP, &SP, pc - 1);
On 2016/06/07 22:31:29, zra wrote:
> I'm not sure I see how the locals from the optimized frame get shuffled into
the
> locals for the unoptimized frame...

If you look at the disassembled optimized code object then you will see that
each deoptimization point comes with a deoptimization environment attached. It
looks like this:

   0: 0x7f64ed3ee6c4  [callerfp][retaddr(1,
6)][pc(2)][const(2)][word(R0)][const(3)][const(4)][const(3)][callerfp][callerpc][pc(1)][const(1)][const(3)]


This is encoding of the stack state in terms of constants and values from the
optimized frame, e.g. [word(R0)] says "take R0 from optimized frame and put it
here".

Deoptimizer removes optimized frame from the stack and them reconstructs
unoptimized frame(s) using this description.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2017: SP = FrameArguments(FP,
materialization_arg_count);
FrameArguments(FP, argc) points to the first argument. Lets look at the stack
layout right after deoptimizer finishes writing it to the stack (can be seen
with --trace-deoptimization-verbose)

*0. [0x2fcf3c8] 0x2fcf390 [callerfp]      \ <- FP
*1. [0x2fcf3c0] 0x0 [retaddr(1, 6)]       |
*2. [0x2fcf3b8] 0x7f64ef3eb171 [pc(2)]    |
*3. [0x2fcf3b0] 0x7f64ef3e8021 [const(2)] /
*4. [0x2fcf3a8] 0x7f64ee014981 [word(R0)] \
*5. [0x2fcf3a0] 0x7f64ee053781 [const(3)] |
*6. [0x2fcf398] 0x7f64ee0ec051 [const(4)] |
*7. [0x2fcf390] 0x7f64ee053781 [const(3)] |
*8. [0x2fcf388] 0x2fcf348 [callerfp]      |
*9. [0x2fcf380] 0x7f64ed3ee3d0 [callerpc] |
*10. [0x2fcf378] 0x7f64ed3c71e1 [pc(1)]   |
*11. [0x2fcf370] 0x7f64ed6b35c1 [const(1)]|
*12. [0x2fcf368] 0x7f64ee053781 [const(3)]/

Slots 0 - 3 are synthetic frame (needed for GC functioning) slots 4 to 12 are
our newly reconstructed unoptimized frame. 

FP is pointing to the slot 0. materialization_arg_count is 0.

If you do SP = FrameArguments(FP, 0) you get SP pointing to the slot 3. 

This would be fine if we were doing a lazy deopt - because we are going to set
SP[0] to result of the function call and continue. 

However for eager deopt we are not going to push any result. 

This means we should set SP to slot 4, not slot 3. Because slot 4 is precisely
where stack frame for unoptimized code ends.

So code should say

SP = FrameArguments(FP, materialization_arg_count + (preserve_result ? 0 : 1))

to restore the correct SP.

[Vyacheslav Egorov (Google), 2016-06-08 11:58:53]

There are two minor off-by-1 in the eager deoptimization code. 

I highlighted both. If they are fixed - deoptimization should start working. 

I tried to explain them.

Please let me know if my explanation helped.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/deopt_instructio...
File runtime/vm/deopt_instructions.cc (left):

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/deopt_instructio...
runtime/vm/deopt_instructions.cc:75: + ((frame->sp() - frame->fp()) / kWordSize)
 // For frame size incl. sp.
On 2016/06/07 22:31:28, zra wrote:
> For some reason sp() is < fp() ? Moved around to at least avoid getting
> something totally wrong from unsigned arithmetic cast to signed.

If SP is less then FP on DBC then this is bug somewhere else.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_dbc.cc:212: // Calls do not need stubs, they
share a deoptimization trampoline.
I don't think we really need DeoptInfoWithStub on DBC. At least not as much as
on other platforms. Instead of doing:

Condition 
Jump stub

We just should do 

Condition 
Deopt ...

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

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:913: __ Push(locs()->in(0).reg());
On 2016/06/07 22:31:28, zra wrote:
> Thoughts about adding an instruction for this that just checks the tag?

Yes, I think this should be just a special bytecode instruction.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:917: __ Jump(deopt);
__ Jump(deopt) could just be __ Deopt(...)

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

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1975: RawObject* result = SP[0];
When not preserving result (non-lazy deopt) we need to preserve SP[0] - it does
not contain result, but contains some temporary expression. So you need to do

if (!preserve_result) SP++; 

before entering the synthetic frame, otherwise SP[0] will be damaged.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1980: EnterSyntheticFrame(&FP, &SP, pc - 1);
On 2016/06/07 22:31:29, zra wrote:
> I'm not sure I see how the locals from the optimized frame get shuffled into
the
> locals for the unoptimized frame...

If you look at the disassembled optimized code object then you will see that
each deoptimization point comes with a deoptimization environment attached. It
looks like this:

   0: 0x7f64ed3ee6c4  [callerfp][retaddr(1,
6)][pc(2)][const(2)][word(R0)][const(3)][const(4)][const(3)][callerfp][callerpc][pc(1)][const(1)][const(3)]


This is encoding of the stack state in terms of constants and values from the
optimized frame, e.g. [word(R0)] says "take R0 from optimized frame and put it
here".

Deoptimizer removes optimized frame from the stack and them reconstructs
unoptimized frame(s) using this description.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2017: SP = FrameArguments(FP,
materialization_arg_count);
FrameArguments(FP, argc) points to the first argument. Lets look at the stack
layout right after deoptimizer finishes writing it to the stack (can be seen
with --trace-deoptimization-verbose)

*0. [0x2fcf3c8] 0x2fcf390 [callerfp]      \ <- FP
*1. [0x2fcf3c0] 0x0 [retaddr(1, 6)]       |
*2. [0x2fcf3b8] 0x7f64ef3eb171 [pc(2)]    |
*3. [0x2fcf3b0] 0x7f64ef3e8021 [const(2)] /
*4. [0x2fcf3a8] 0x7f64ee014981 [word(R0)] \
*5. [0x2fcf3a0] 0x7f64ee053781 [const(3)] |
*6. [0x2fcf398] 0x7f64ee0ec051 [const(4)] |
*7. [0x2fcf390] 0x7f64ee053781 [const(3)] |
*8. [0x2fcf388] 0x2fcf348 [callerfp]      |
*9. [0x2fcf380] 0x7f64ed3ee3d0 [callerpc] |
*10. [0x2fcf378] 0x7f64ed3c71e1 [pc(1)]   |
*11. [0x2fcf370] 0x7f64ed6b35c1 [const(1)]|
*12. [0x2fcf368] 0x7f64ee053781 [const(3)]/

Slots 0 - 3 are synthetic frame (needed for GC functioning) slots 4 to 12 are
our newly reconstructed unoptimized frame. 

FP is pointing to the slot 0. materialization_arg_count is 0.

If you do SP = FrameArguments(FP, 0) you get SP pointing to the slot 3. 

This would be fine if we were doing a lazy deopt - because we are going to set
SP[0] to result of the function call and continue. 

However for eager deopt we are not going to push any result. 

This means we should set SP to slot 4, not slot 3. Because slot 4 is precisely
where stack frame for unoptimized code ends.

So code should say

SP = FrameArguments(FP, materialization_arg_count + (preserve_result ? 0 : 1))

to restore the correct SP.

[zra, 2016-06-08 17:46:31]

The fixes worked =)

PTAL

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/deopt_instructio...
File runtime/vm/deopt_instructions.cc (left):

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/deopt_instructio...
runtime/vm/deopt_instructions.cc:75: + ((frame->sp() - frame->fp()) / kWordSize)
 // For frame size incl. sp.
On 2016/06/08 11:58:53, Vyacheslav Egorov (Google) wrote:
> On 2016/06/07 22:31:28, zra wrote:
> > For some reason sp() is < fp() ? Moved around to at least avoid getting
> > something totally wrong from unsigned arithmetic cast to signed.
> 
> If SP is less then FP on DBC then this is bug somewhere else.

Added ASSERT.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_dbc.cc (right):

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_dbc.cc:212: // Calls do not need stubs, they
share a deoptimization trampoline.
On 2016/06/08 11:58:53, Vyacheslav Egorov (Google) wrote:
> I don't think we really need DeoptInfoWithStub on DBC. At least not as much as
> on other platforms. Instead of doing:
> 
> Condition 
> Jump stub
> 
> We just should do 
> 
> Condition 
> Deopt ... 
> 
> 

Done.

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

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:913: __ Push(locs()->in(0).reg());
On 2016/06/08 11:58:53, Vyacheslav Egorov (Google) wrote:
> On 2016/06/07 22:31:28, zra wrote:
> > Thoughts about adding an instruction for this that just checks the tag?
> 
> Yes, I think this should be just a special bytecode instruction.

Done.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_dbc.cc:917: __ Jump(deopt);
On 2016/06/08 11:58:53, Vyacheslav Egorov (Google) wrote:
> __ Jump(deopt) could just be __ Deopt(...)

Done.

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

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1975: RawObject* result = SP[0];
On 2016/06/08 11:58:53, Vyacheslav Egorov (Google) wrote:
> When not preserving result (non-lazy deopt) we need to preserve SP[0] - it
does
> not contain result, but contains some temporary expression. So you need to do
> 
> if (!preserve_result) SP++; 
> 
> before entering the synthetic frame, otherwise SP[0] will be damaged.
> 

This and the fix below made things work.

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:1980: EnterSyntheticFrame(&FP, &SP, pc - 1);
On 2016/06/08 11:58:53, Vyacheslav Egorov (Google) wrote:
> On 2016/06/07 22:31:29, zra wrote:
> > I'm not sure I see how the locals from the optimized frame get shuffled into
> the
> > locals for the unoptimized frame...
> 
> If you look at the disassembled optimized code object then you will see that
> each deoptimization point comes with a deoptimization environment attached. It
> looks like this:
> 
>    0: 0x7f64ed3ee6c4  [callerfp][retaddr(1,
>
6)][pc(2)][const(2)][word(R0)][const(3)][const(4)][const(3)][callerfp][callerpc][pc(1)][const(1)][const(3)]
> 
> 
> This is encoding of the stack state in terms of constants and values from the
> optimized frame, e.g. [word(R0)] says "take R0 from optimized frame and put it
> here".
> 
> Deoptimizer removes optimized frame from the stack and them reconstructs
> unoptimized frame(s) using this description. 

Thanks!

https://codereview.chromium.org/2039913006/diff/1/runtime/vm/simulator_dbc.cc...
runtime/vm/simulator_dbc.cc:2017: SP = FrameArguments(FP,
materialization_arg_count);
On 2016/06/08 11:58:53, Vyacheslav Egorov (Google) wrote:
> FrameArguments(FP, argc) points to the first argument. Lets look at the stack
> layout right after deoptimizer finishes writing it to the stack (can be seen
> with --trace-deoptimization-verbose)
> 
> *0. [0x2fcf3c8] 0x2fcf390 [callerfp]      \ <- FP
> *1. [0x2fcf3c0] 0x0 [retaddr(1, 6)]       |
> *2. [0x2fcf3b8] 0x7f64ef3eb171 [pc(2)]    |
> *3. [0x2fcf3b0] 0x7f64ef3e8021 [const(2)] /
> *4. [0x2fcf3a8] 0x7f64ee014981 [word(R0)] \
> *5. [0x2fcf3a0] 0x7f64ee053781 [const(3)] |
> *6. [0x2fcf398] 0x7f64ee0ec051 [const(4)] |
> *7. [0x2fcf390] 0x7f64ee053781 [const(3)] |
> *8. [0x2fcf388] 0x2fcf348 [callerfp]      |
> *9. [0x2fcf380] 0x7f64ed3ee3d0 [callerpc] |
> *10. [0x2fcf378] 0x7f64ed3c71e1 [pc(1)]   |
> *11. [0x2fcf370] 0x7f64ed6b35c1 [const(1)]|
> *12. [0x2fcf368] 0x7f64ee053781 [const(3)]/
> 
> Slots 0 - 3 are synthetic frame (needed for GC functioning) slots 4 to 12 are
> our newly reconstructed unoptimized frame. 
> 
> FP is pointing to the slot 0. materialization_arg_count is 0.
> 
> If you do SP = FrameArguments(FP, 0) you get SP pointing to the slot 3. 
> 
> This would be fine if we were doing a lazy deopt - because we are going to set
> SP[0] to result of the function call and continue. 
> 
> However for eager deopt we are not going to push any result. 
> 
> This means we should set SP to slot 4, not slot 3. Because slot 4 is precisely
> where stack frame for unoptimized code ends.
> 
> So code should say
> 
> SP = FrameArguments(FP, materialization_arg_count + (preserve_result ? 0 : 1))
> 
> to restore the correct SP.

That makes sense, thanks.

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

lgtm

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

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:72: (static_cast<intptr_t>(frame->sp()) -
I don't think these casts are needed really, they just make things look more
convoluted. 

You can add an assertion sp() < fp() and sp() > fp() for !DBC and DBC
respectively.

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

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1003:
info->set_pc_offset(assembler()->CodeSize());
After looking through deoptimization code I realized where the
set_pc_offset(...) discrepancy between DBC and other architectures was coming
from. See comments in the simulator code.

If you resolve them then you should be able to write

__ Deopt(0, /*is_eager=*/1)
info->set_pc_offset(...);

which matches other architectures.

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

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:914: __ Deopt(0, deopt_id());
I would prefer to write this as 

compiler->EmitDeopt(deopt_id(),
                    ICData::kDeoptCheckSmi,
                    licm_hoisted_ ? ICData::kHoisted : 0);

and have Deopt instruction be generated and pc recorded in a single place.

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

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1977: const uint16_t deopt_id = rD;
My original idea was to use this value to look up deopt information, instead of
searching by PC - but now I realize that it would be too much work.

So we can just change it's meaning to mean 'is eager' because currently it's not
used for anything. 

const bool is_lazy = rD == 0; or 

const bool is_eager = rD != 0;

depending on what you find more readable.

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1978: const bool preserve_result = (deopt_id == 0);
I suggest removing preserve_result and use either is_eager or is_lazy.

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1995: EnterSyntheticFrame(&FP, &SP, pc - 1);
I overlooked that this should say 

pc - is_eager ? 0 : 1.

We are going one backwards for lazy deopts because code looks like this:

.... InstanceCall
.... Deopt
PC->

and deopt information is actually recorded for PC immediately after the call.

However for normal eager deopts we should record deopt information for PC
directly after Deopt instruction so PC-1 is not needed.

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2036: materialization_arg_count + (preserve_result ?
0 : 1));
I thought about it and it might be more readable to write it as 

// SP points to the result slot that used to contain the first argument.
SP = FrameArguments(FP, materialization_arg_count);


...

if (is_eager) {
  SP--;  // No result to push.
} else {
  SP[0] = result;  // Put result to the stack.
}

[zra, 2016-06-09 16:42:23]

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

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:72: (static_cast<intptr_t>(frame->sp()) -
On 2016/06/09 15:09:06, Vyacheslav Egorov (Google) wrote:
> I don't think these casts are needed really, they just make things look more
> convoluted. 
> 
> You can add an assertion sp() < fp() and sp() > fp() for !DBC and DBC
> respectively. 

Done.

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

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1003:
info->set_pc_offset(assembler()->CodeSize());
On 2016/06/09 15:09:06, Vyacheslav Egorov (Google) wrote:
> After looking through deoptimization code I realized where the
> set_pc_offset(...) discrepancy between DBC and other architectures was coming
> from. See comments in the simulator code.
> 
> If you resolve them then you should be able to write
> 
> __ Deopt(0, /*is_eager=*/1)
> info->set_pc_offset(...);
> 
> which matches other architectures.

Done.

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

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_dbc.cc:914: __ Deopt(0, deopt_id());
On 2016/06/09 15:09:06, Vyacheslav Egorov (Google) wrote:
> I would prefer to write this as 
> 
> compiler->EmitDeopt(deopt_id(),
>                     ICData::kDeoptCheckSmi,
>                     licm_hoisted_ ? ICData::kHoisted : 0);
> 
> and have Deopt instruction be generated and pc recorded in a single place.

Done.

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

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1977: const uint16_t deopt_id = rD;
On 2016/06/09 15:09:06, Vyacheslav Egorov (Google) wrote:
> My original idea was to use this value to look up deopt information, instead
of
> searching by PC - but now I realize that it would be too much work.
> 
> So we can just change it's meaning to mean 'is eager' because currently it's
not
> used for anything. 
> 
> const bool is_lazy = rD == 0; or 
> 
> const bool is_eager = rD != 0;
> 
> depending on what you find more readable.

Done.

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1978: const bool preserve_result = (deopt_id == 0);
On 2016/06/09 15:09:06, Vyacheslav Egorov (Google) wrote:
> I suggest removing preserve_result and use either is_eager or is_lazy.

Done.

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:1995: EnterSyntheticFrame(&FP, &SP, pc - 1);
On 2016/06/09 15:09:06, Vyacheslav Egorov (Google) wrote:
> I overlooked that this should say 
> 
> pc - is_eager ? 0 : 1.
> 
> We are going one backwards for lazy deopts because code looks like this:
> 
> .... InstanceCall
> .... Deopt
> PC->
> 
> and deopt information is actually recorded for PC immediately after the call.
> 
> However for normal eager deopts we should record deopt information for PC
> directly after Deopt instruction so PC-1 is not needed.

Done.

https://codereview.chromium.org/2039913006/diff/60001/runtime/vm/simulator_db...
runtime/vm/simulator_dbc.cc:2036: materialization_arg_count + (preserve_result ?
0 : 1));
On 2016/06/09 15:09:06, Vyacheslav Egorov (Google) wrote:
> I thought about it and it might be more readable to write it as 
> 
> // SP points to the result slot that used to contain the first argument.
> SP = FrameArguments(FP, materialization_arg_count);
> 
> 
> ...
> 
> if (is_eager) {
>   SP--;  // No result to push.
> } else {
>   SP[0] = result;  // Put result to the stack.
> }
>  

Done.

[zra, 2016-06-09 16:47:59]

Description was changed from

==========
DBC: Eager deoptimization and CheckSmi instruction.
==========

to

==========
DBC: Eager deoptimization and CheckSmi instruction.

R=vegorov@google.com

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

[zra, 2016-06-09 16:48:01]

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