Reapply "Shuffle around deopt id allocation...

Reapply "Shuffle around deopt id allocation to give the flow graph builder a chance to record other data as they are allocated".

 - Fix inlining of new List(n) to propogate deopt id.
 - Fix CreateArrayOpt to check for a Smi length.

R=vegorov@google.com

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

[rmacnak, 2017-05-24 17:05:05]

Description was changed from

==========
DBC: Account for calls without deopt ids.
==========

to

==========
DBC: Account for calls without deopt ids.

Cf. c94bebf28937e5250b2d6486503848645be142b1
==========

[rmacnak, 2017-05-24 17:05:44]

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

[Vyacheslav Egorov (Google), 2017-05-24 19:14:05]

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

https://codereview.chromium.org/2900963008/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_dbc.cc:193: intptr_t deopt_id_after;
This is a bit suspicious. What kind of instruction contains a call inside but
does not have a deopt_id?

If this call can trigger lazy deoptimization it *must* have a deopt id otherwise
lazy deoptimization will fail.

[rmacnak, 2017-05-24 19:17:31]

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

https://codereview.chromium.org/2900963008/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_dbc.cc:193: intptr_t deopt_id_after;
On 2017/05/24 19:14:05, Vyacheslav Egorov (Google) wrote:
> This is a bit suspicious. What kind of instruction contains a call inside but
> does not have a deopt_id?
> 
> If this call can trigger lazy deoptimization it *must* have a deopt id
otherwise
> lazy deoptimization will fail. 

A CreateArrayInstr. The other architectures are factored differently so I had
allowed specifically CreateArrayInstr to have no deopt id when optimizing.
Another way to address this would be to have CreateArrayInstr check for no deopt
id on DBC as well.

[Vyacheslav Egorov (Google), 2017-05-24 19:25:17]

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

https://codereview.chromium.org/2900963008/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_dbc.cc:193: intptr_t deopt_id_after;
On 2017/05/24 19:17:31, rmacnak wrote:
> On 2017/05/24 19:14:05, Vyacheslav Egorov (Google) wrote:
> > This is a bit suspicious. What kind of instruction contains a call inside
but
> > does not have a deopt_id?
> > 
> > If this call can trigger lazy deoptimization it *must* have a deopt id
> otherwise
> > lazy deoptimization will fail. 
> 
> A CreateArrayInstr. The other architectures are factored differently so I had
> allowed specifically CreateArrayInstr to have no deopt id when optimizing.
> Another way to address this would be to have CreateArrayInstr check for no
deopt
> id on DBC as well.

I think it is possible to cause lazy deoptimization with CreateArray because it
invokes exception constructors, so it is not safe to omit deopt_id for it. 

Let me try to make a repro.

[Vyacheslav Egorov (Google), 2017-05-24 19:50:33]

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

https://codereview.chromium.org/2900963008/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_dbc.cc:193: intptr_t deopt_id_after;
On 2017/05/24 19:25:17, Vyacheslav Egorov (Google) wrote:
> On 2017/05/24 19:17:31, rmacnak wrote:
> > On 2017/05/24 19:14:05, Vyacheslav Egorov (Google) wrote:
> > > This is a bit suspicious. What kind of instruction contains a call inside
> but
> > > does not have a deopt_id?
> > > 
> > > If this call can trigger lazy deoptimization it *must* have a deopt id
> > otherwise
> > > lazy deoptimization will fail. 
> > 
> > A CreateArrayInstr. The other architectures are factored differently so I
had
> > allowed specifically CreateArrayInstr to have no deopt id when optimizing.
> > Another way to address this would be to have CreateArrayInstr check for no
> deopt
> > id on DBC as well.
> 
> I think it is possible to cause lazy deoptimization with CreateArray because
it
> invokes exception constructors, so it is not safe to omit deopt_id for it. 
> 
> Let me try to make a repro.

Actually these days a simplest repro like this would do it - because we use
deoptimization to materialize state for exception handler (see
GetMetaDataFromDeopt in exceptions.cc - the stuff that Dmitry implemented).

[This repro also triggers crash in the simulator_dbc.cc becuase CreateArrayOpt
does not account for possibility of non-smi array lengthes - which I guess need
to be fixed separately]

 const NeverInline = "NeverInline";

 @NeverInline
 foo(int n) {
   try {
     return new List(n);
   } catch (e) {
   }
 }

 main () {
   for (var i = 0; i < 100000; i++) foo(5);
   foo("");
 }

[rmacnak, 2017-05-24 22:34:19]

Description was changed from

==========
DBC: Account for calls without deopt ids.

Cf. c94bebf28937e5250b2d6486503848645be142b1
==========

to

==========
Reapply "Shuffle around deopt id allocation to give the flow graph builder a
chance to record other data as they are allocated".
    
 - Fix inlining of new List(n) to propogate deopt id.
 - Fix CreateArrayOpt check for Smi length.
==========

[rmacnak, 2017-05-24 22:38:40]

Description was changed from

==========
Reapply "Shuffle around deopt id allocation to give the flow graph builder a
chance to record other data as they are allocated".
    
 - Fix inlining of new List(n) to propogate deopt id.
 - Fix CreateArrayOpt check for Smi length.
==========

to

==========
Reapply "Shuffle around deopt id allocation to give the flow graph builder a
chance to record other data as they are allocated".
    
 - Fix inlining of new List(n) to propogate deopt id.
 - Fix CreateArrayOpt to check for a Smi length.
==========

[rmacnak, 2017-05-24 22:59:34]

https://codereview.chromium.org/2900963008/diff/40001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2900963008/diff/40001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:3704: *last = new (Z)
CreateArrayInstr(call->token_pos(), type,
Actually it looks like we've just been neglecting to forward the deopt id here.

[Vyacheslav Egorov (Google), 2017-05-25 08:44:04]

lgtm

https://codereview.chromium.org/2900963008/diff/40001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2900963008/diff/40001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:3704: *last = new (Z)
CreateArrayInstr(call->token_pos(), type,
On 2017/05/24 22:59:34, rmacnak wrote:
> Actually it looks like we've just been neglecting to forward the deopt id
here.

Good catch! Sorry I missed it in the original review - my eyes failed to see
this in the avalanche of TargetEntryInstr.

We had a refactoring planned long ago that would have caught this: make deopt_id
and Environment* use the same slot in the instruction (because environment
already contains a deopt_id). Currently instruction has both, so one is
redundant.

In optimizing compiler we only need env and in unoptimizing we only need
deopt_id.

[rmacnak, 2017-05-25 17:12:27]

Description was changed from

==========
Reapply "Shuffle around deopt id allocation to give the flow graph builder a
chance to record other data as they are allocated".
    
 - Fix inlining of new List(n) to propogate deopt id.
 - Fix CreateArrayOpt to check for a Smi length.
==========

to

==========
Reapply "Shuffle around deopt id allocation to give the flow graph builder a
chance to record other data as they are allocated".

 - Fix inlining of new List(n) to propogate deopt id.
 - Fix CreateArrayOpt to check for a Smi length.

R=vegorov@google.com

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

[rmacnak, 2017-05-25 17:12:28]

Committed patchset #3 (id:40001) manually as
bcf2900a262da7da1b0514ff2d2e0fbe22fa5e1c (presubmit successful).