Inlining of static calls with trivial function bodies.

Inlining of static calls with trivial function bodies.

R=kmillikin@google.com,srdjan@google.com

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

[zerny-google, 2012-08-29 12:24:29]

Here is the first stab at inlining in the simple case of static calls of
functions with trivial bodies (sequence, return and literals). I look forward to
your comments.

[Kevin Millikin (Google), 2012-08-29 14:01:04]

LGTM as a start.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:42: DEFINE_FLAG(bool, inlining, true, "Enable call-site
inlining");
Flag name should be a verb to match the others.  Suggest: "inline" or
"use_inlining".

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:695: // Helper to get the block-entry ancestor of an
instruction.
'ancestor' makes me think of trees.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.h
File runtime/vm/flow_graph.h (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.h#new...
runtime/vm/flow_graph.h:74: void ComputeSSAForInlining(intptr_t
callers_max_virtual_register_number);
I'd call this something like 'next_virtual_register_number', pass it to
ComputeSSA, and get rid of the extra function.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.h (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.h:158: // TODO(zerny): This is assuming we
actually use all visitors.
Not really a TODO, just an assumption.

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

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:7: #include "vm/object.h"
These are normally sorted alphabetically (M-x sort-lines).

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:28: 
Too many blank lines.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:69: OS::Print("Callee graph for inlining %s\n",
Might be useful to write 'before SSA' here and 'after SSA' later.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:134: // if (FLAG_trace_inlining)
OS::Print("Polymorphic call\n");
Remove the commented-out code.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:150: void VisitClosureCall(ClosureCallComp*
comp, BindInstr* instr) {
Remove all this code for now, too.

[srdjan, 2012-08-29 21:31:33]

LGTM with comments and what Kevin said.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:702: // Helper to link two instruction in the graph.
s/instruction/instructions/

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.h (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.h:60: bool inlining() const { return
inlining_context_ != kNotInlining; }
Does this means: can_be_inlined or does it mean that it can inline others?

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

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:66: }
You may also want to bailout if the callee has named arguments, since you do not
know if the order at caller matches.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:138: // if (FLAG_trace_inlining)
OS::Print("Instance call\n");
remove dead code.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:181: }
Closure calls are rare in the benchmarks targeted. I would focus rather on
PolymorphicInstanceCall instead.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:197: return;
Use curly braces if if-statement needs more than one line.

[zerny-google, 2012-08-30 07:31:40]

Thanks for the reviews. I have addressed the comments and will dcommit shortly.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:42: DEFINE_FLAG(bool, inlining, true, "Enable call-site
inlining");
On 2012/08/29 14:01:04, kmillikin wrote:
> Flag name should be a verb to match the others.  Suggest: "inline" or
> "use_inlining".

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:695: // Helper to get the block-entry ancestor of an
instruction.
On 2012/08/29 14:01:04, kmillikin wrote:
> 'ancestor' makes me think of trees.

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:702: // Helper to link two instruction in the graph.
On 2012/08/29 21:31:33, srdjan wrote:
> s/instruction/instructions/

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.h
File runtime/vm/flow_graph.h (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph.h#new...
runtime/vm/flow_graph.h:74: void ComputeSSAForInlining(intptr_t
callers_max_virtual_register_number);
On 2012/08/29 14:01:04, kmillikin wrote:
> I'd call this something like 'next_virtual_register_number', pass it to
> ComputeSSA, and get rid of the extra function.

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.h (right):

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.h:60: bool inlining() const { return
inlining_context_ != kNotInlining; }
On 2012/08/29 21:31:33, srdjan wrote:
> Does this means: can_be_inlined or does it mean that it can inline others?

It signals that the builder is building a graph to be inlined. 

Currently the inlining context is only used to conditionally bailout of the
builder when inlining. Later we will need to distinguish between the actual
inlining context.

Renaming s/inlining/InInliningContext to make the meaning clear.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.h:158: // TODO(zerny): This is assuming we
actually use all visitors.
On 2012/08/29 14:01:04, kmillikin wrote:
> Not really a TODO, just an assumption.

Done.

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

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:7: #include "vm/object.h"
On 2012/08/29 14:01:04, kmillikin wrote:
> These are normally sorted alphabetically (M-x sort-lines).

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:28: 
On 2012/08/29 14:01:04, kmillikin wrote:
> Too many blank lines.

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:66: }
On 2012/08/29 21:31:33, srdjan wrote:
> You may also want to bailout if the callee has named arguments, since you do
not
> know if the order at caller matches.

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:69: OS::Print("Callee graph for inlining %s\n",
On 2012/08/29 14:01:04, kmillikin wrote:
> Might be useful to write 'before SSA' here and 'after SSA' later.

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:134: // if (FLAG_trace_inlining)
OS::Print("Polymorphic call\n");
On 2012/08/29 14:01:04, kmillikin wrote:
> Remove the commented-out code.

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:138: // if (FLAG_trace_inlining)
OS::Print("Instance call\n");
On 2012/08/29 21:31:33, srdjan wrote:
> remove dead code.

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:150: void VisitClosureCall(ClosureCallComp*
comp, BindInstr* instr) {
On 2012/08/29 14:01:04, kmillikin wrote:
> Remove all this code for now, too.

Done.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:181: }
On 2012/08/29 21:31:33, srdjan wrote:
> Closure calls are rare in the benchmarks targeted. I would focus rather on
> PolymorphicInstanceCall instead.

Ok.

http://codereview.chromium.org/10893027/diff/4001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:197: return;
On 2012/08/29 21:31:33, srdjan wrote:
> Use curly braces if if-statement needs more than one line.

Done.