Deoptimization support in inlined code.

Deoptimization support in inlined code.

Committed: https://code.google.com/p/dart/source/detail?r=12488

[zerny-google, 2012-09-18 10:37:27]

Added support for deoptimization inside inlined code.

[Kevin Millikin (Google), 2012-09-18 11:01:55]

LGTM.  Feel free to address the comments as a separate change.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/compiler.h#newco...
runtime/vm/compiler.h:62: static RawArray* ExtractTypeFeedbackArray(const Code&
code);
It seems better to move this to a member function on class Code.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/deopt_instructio...
File runtime/vm/deopt_instructions.h (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/deopt_instructio...
runtime/vm/deopt_instructions.h:29: intptr_t* GetFromFrameAddressAt(intptr_t
index) const {
I don't think we care about the address in the from frame.  We might consider
also making this into "intptr_t GetFromFrameValueAt(intptr_t index)...".

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_alloc...
File runtime/vm/flow_graph_allocator.cc (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:745: if (env->outer() != NULL) {
The base of the recursion handles env == NULL, so no need to test (or: you can
assert env is not NULL instead of checking for NULL an returning).

It is probably better to turn this into a while loop, since it doesn't seem to
matter which order we process the environments.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.cc (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:49: // with PushArgument.  Recursively
allocates from outer most to inner most
"outer most" ==> "outermost", "inner most" ==> "innermost".

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:81: // For functions with optional arguments,
all incoming are copied to local
Hmm.  I would say they're "above" the frame pointer on the stack (e.g., at a
lower address).  I tend to avoid above and below, so you could say they are
copied to spill slots.

I don't understand the part about tracking in the deoptimization environment. 
They are tracked, as part of the spill slots, right?

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:86: // For the inner-most environment call the
virutal return builder.
"virutal" ==> "virtual".

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.h (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.h:74: intptr_t* stack_height);
Spell check :)

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
File runtime/vm/flow_graph_inliner.cc (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:12: #include "vm/intrinsifier.h"
I think we try to keep these sorted according to M-x sort-lines (e.g., intrin...
after il_pri...").

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:64: const intptr_t prev_deopt_id =
isolate->deopt_id();
It distracts me that we have old_ic_data and prev_deopt_id.  They should both be
old or both be prev.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:99: // Compute SSA on the callee graph.
(catching bailouts)
Format as a complete sentence: "Compute SSA on the callee graph, catching
bailouts."

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:103: // TODO(zerny): Do optimization passes on
the callee graph.
"Do more optimization...."

[Kevin Millikin (Google), 2012-09-18 11:02:27]

On 2012/09/18 11:01:55, kmillikin wrote:
> LGTM.  Feel free to address the comments as a separate change.
> 

Well, go ahead and address the spelling fixes immediately :)

[zerny-google, 2012-09-18 11:53:07]

Updated according to your review and will do the deopt_instruction changes in a
separate CL.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/compiler.h#newco...
runtime/vm/compiler.h:62: static RawArray* ExtractTypeFeedbackArray(const Code&
code);
On 2012/09/18 11:01:55, kmillikin wrote:
> It seems better to move this to a member function on class Code.

Done.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/deopt_instructio...
File runtime/vm/deopt_instructions.h (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/deopt_instructio...
runtime/vm/deopt_instructions.h:29: intptr_t* GetFromFrameAddressAt(intptr_t
index) const {
On 2012/09/18 11:01:55, kmillikin wrote:
> I don't think we care about the address in the from frame.  We might consider
> also making this into "intptr_t GetFromFrameValueAt(intptr_t index)...".

Will do in separate CL.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_alloc...
File runtime/vm/flow_graph_allocator.cc (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_alloc...
runtime/vm/flow_graph_allocator.cc:745: if (env->outer() != NULL) {
On 2012/09/18 11:01:55, kmillikin wrote:
> The base of the recursion handles env == NULL, so no need to test (or: you can
> assert env is not NULL instead of checking for NULL an returning).
> 
> It is probably better to turn this into a while loop, since it doesn't seem to
> matter which order we process the environments.

Done.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.cc (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:49: // with PushArgument.  Recursively
allocates from outer most to inner most
On 2012/09/18 11:01:55, kmillikin wrote:
> "outer most" ==> "outermost", "inner most" ==> "innermost".

Done.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:81: // For functions with optional arguments,
all incoming are copied to local
On 2012/09/18 11:01:55, kmillikin wrote:
> Hmm.  I would say they're "above" the frame pointer on the stack (e.g., at a
> lower address).  I tend to avoid above and below, so you could say they are
> copied to spill slots.
> 
> I don't understand the part about tracking in the deoptimization environment. 
> They are tracked, as part of the spill slots, right?

Also, the this comment is for num_incoming_args (renamed incoming_args_count).

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:86: // For the inner-most environment call the
virutal return builder.
On 2012/09/18 11:01:55, kmillikin wrote:
> "virutal" ==> "virtual".

Done.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.h (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.h:74: intptr_t* stack_height);
On 2012/09/18 11:01:55, kmillikin wrote:
> Spell check :)

Thanks! Done.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
File runtime/vm/flow_graph_inliner.cc (right):

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:12: #include "vm/intrinsifier.h"
On 2012/09/18 11:01:55, kmillikin wrote:
> I think we try to keep these sorted according to M-x sort-lines (e.g.,
intrin...
> after il_pri...").

Thanks again.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:64: const intptr_t prev_deopt_id =
isolate->deopt_id();
On 2012/09/18 11:01:55, kmillikin wrote:
> It distracts me that we have old_ic_data and prev_deopt_id.  They should both
be
> old or both be prev.

Done.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:99: // Compute SSA on the callee graph.
(catching bailouts)
On 2012/09/18 11:01:55, kmillikin wrote:
> Format as a complete sentence: "Compute SSA on the callee graph, catching
> bailouts."

Done.

http://codereview.chromium.org/10928232/diff/7001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:103: // TODO(zerny): Do optimization passes on
the callee graph.
On 2012/09/18 11:01:55, kmillikin wrote:
> "Do more optimization...."

Done.