Store optimized flow graph statistics on the function itself.

Store optimized flow graph statistics on the function itself.

This gives a good estimate for an early bailout from the inlining attempt.

BUG=

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

[Vyacheslav Egorov (Google), 2012-12-13 16:27:53]

FYI

[Kevin Millikin (Google), 2012-12-13 16:43:40]

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

https://codereview.chromium.org/11567012/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:188: FlowGraphInliner::CollectGraphInfo(flow_graph);
Why not move this down into FlowGraphInliner::Inline()?  Just to avoid timing
it?

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

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:408: const intptr_t loop_depth =
call->GetBlock()->loop_depth();
Move this above the first call to ShouldWeInline.

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:486: intptr_t constants_count = 0;
There's no reason to count this again, is there?

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:494: if (!ShouldWeInline(loop_depth,
It seems strange to call this twice?  Is it needed?

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:505:
function.set_optimized_instruction_count(size);
Also a bit strange to set this only when we decide not to inline.  Can't it
communicate useful information if we succeeded?  (E.g., we might succeed here
due to hotness or loop nesting, but fail at other sites).

[Vyacheslav Egorov (Google), 2012-12-13 18:54:46]

PTAL

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

https://codereview.chromium.org/11567012/diff/1/runtime/vm/compiler.cc#newcod...
runtime/vm/compiler.cc:188: FlowGraphInliner::CollectGraphInfo(flow_graph);
On 2012/12/13 16:43:40, kmillikin wrote:
> Why not move this down into FlowGraphInliner::Inline()?  Just to avoid timing
> it?

Done.

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

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:408: const intptr_t loop_depth =
call->GetBlock()->loop_depth();
On 2012/12/13 16:43:40, kmillikin wrote:
> Move this above the first call to ShouldWeInline.

Done.

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:486: intptr_t constants_count = 0;
On 2012/12/13 16:43:40, kmillikin wrote:
> There's no reason to count this again, is there?

Actually the count above is just an approximation because arguments might be
padded with default values for optional parameters to obtain param_stubs.

And I can't use the same helper here because param_stubs are definitions and
arguments are values.

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:494: if (!ShouldWeInline(loop_depth,
On 2012/12/13 16:43:40, kmillikin wrote:
> It seems strange to call this twice?  Is it needed?

Yes. Sometimes we attempt to inline big function that was not optimized itself
yet and thus we don't have any information about its counters so they are all
zeros (and thus we don't get an early bailout).

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:505:
function.set_optimized_instruction_count(size);
On 2012/12/13 16:43:40, kmillikin wrote:
> Also a bit strange to set this only when we decide not to inline.  Can't it
> communicate useful information if we succeeded?  (E.g., we might succeed here
> due to hotness or loop nesting, but fail at other sites).

Yep, this is wrong.

[srdjan, 2012-12-13 19:00:17]

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

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:360: TRACE_INLINING(OS::Print("     Bailout:
early heuristics\n"));
Maybe print also the arguments that led to inlining bailout.

https://codereview.chromium.org/11567012/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11567012/diff/1/runtime/vm/object.h#newcode1291
runtime/vm/object.h:1291: uint16_t optimized_instruction_count() const {
I think it would be better to pass and return intptr_t-s instead of uint16_t-s.
That would also require a check in the setter that clamps the counter to max
uint16_t value and aserts that it is not less than 0.

[Vyacheslav Egorov (Google), 2012-12-13 19:14:11]

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

https://codereview.chromium.org/11567012/diff/1/runtime/vm/flow_graph_inliner...
runtime/vm/flow_graph_inliner.cc:360: TRACE_INLINING(OS::Print("     Bailout:
early heuristics\n"));
On 2012/12/13 19:00:17, srdjan wrote:
> Maybe print also the arguments that led to inlining bailout.

Done.

https://codereview.chromium.org/11567012/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11567012/diff/1/runtime/vm/object.h#newcode1291
runtime/vm/object.h:1291: uint16_t optimized_instruction_count() const {
On 2012/12/13 19:00:17, srdjan wrote:
> I think it would be better to pass and return intptr_t-s instead of
uint16_t-s.
> That would also require a check in the setter that clamps the counter to max
> uint16_t value and aserts that it is not less than 0.

Done.

[srdjan, 2012-12-13 19:16:41]

lgtm

[srdjan, 2012-12-13 19:16:46]

lgtm