Inline monomorphic calls.

Inline monomorphic calls.

Also add inlining support for more instructions that can not deoptimize.

Original changelist: http://codereview.chromium.org/10913010/

BUG=

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

[Kevin Millikin (Google), 2012-09-11 13:28:13]

Original change from Ian.

[Florian Schneider, 2012-09-11 14:36:09]

https://chromiumcodereview.appspot.com/10916228/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_builder.cc (right):

https://chromiumcodereview.appspot.com/10916228/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:243:
InlineBailout("EffectGraphVisitor::BuildStoreLocal (deopt)");
Why this bailout?

https://chromiumcodereview.appspot.com/10916228/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.cc:267:
InlineBailout("EffectGraphVisitor::BuildLoadLocal (deopt)");
Why this bailout? LoadVMField and CurrentContext can't deoptimize.

https://chromiumcodereview.appspot.com/10916228/diff/1/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_inliner.cc (right):

https://chromiumcodereview.appspot.com/10916228/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_inliner.cc:176: void VisitInstanceCall(InstanceCallInstr*
call) {
I think it's not necessary to have VisitInstanceCall. All instance calls that
have IC data and a monomorphic target should have been transformed into
CheckClass + PolymorphicInstanceCall instructions previously by the optimizer.

By doing this maybe you can also avoid checking the IC data again for the same
things that are done in FlowGraphOptimizer::VisitInstanceCall.

https://chromiumcodereview.appspot.com/10916228/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_inliner.cc:216: if
(call->ic_data()->GetReceiverClassIdAt(0) == kSmiCid) {
Maybe it's cleaner for now to not handle smi receivers here.

https://chromiumcodereview.appspot.com/10916228/diff/1/runtime/vm/flow_graph_...
runtime/vm/flow_graph_inliner.cc:221:
ICData::ZoneHandle(call->ic_data()->AsUnaryClassChecks());
I'd abort inlining if the target in not unique:

if (!HasOneTarget(unary_checks)) return;

This is also done in FlowGraphOptimizer::VisitInstanceCall.

[Florian Schneider, 2012-09-11 14:37:26]

Otherwise LGTM.

[srdjan, 2012-09-11 14:44:07]

LGTM with comments

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

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph.cc#newco...
runtime/vm/flow_graph.cc:782: if (callee_exits->length() == 0) {
is_empty()

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (right):

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_builder....
runtime/vm/flow_graph_builder.cc:69: ASSERT(!owner()->InInliningContext() ||
!definition->CanDeoptimize());
These asserts prevent inlining of definitions that can deoptimize, don't they?

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

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:32: Definition* check) {
Maybe describe briefly what is 'check' used for in comment.

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:37: // Abort if the callee has named
parameters.
why change named -> optionals, as optional is used everywhere below?

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:129: // Replace callees null constant with
callers null constant.
s/callees/callee's/

ditto callers

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:197: // TODO(zerny): proceed if each check has
the same cid?
s/cid/target/ ?

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:236:
TryInliningInstanceCall(call->instance_call(), call);
PolymorphicInstanceCall has a with_checks tester. If it returns false, no check
should be emitted.

[Kevin Millikin (Google), 2012-09-11 16:29:20]

Incorporated comments, removed the unnecessary check.  Please take another quick
look.

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

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph.cc#newco...
runtime/vm/flow_graph.cc:782: if (callee_exits->length() == 0) {
On 2012/09/11 14:44:07, srdjan wrote:
> is_empty()

Thank you.

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (right):

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_builder....
runtime/vm/flow_graph_builder.cc:69: ASSERT(!owner()->InInliningContext() ||
!definition->CanDeoptimize());
On 2012/09/11 14:44:07, srdjan wrote:
> These asserts prevent inlining of definitions that can deoptimize, don't they?

For now it catches if we try.

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_builder....
runtime/vm/flow_graph_builder.cc:243:
InlineBailout("EffectGraphVisitor::BuildStoreLocal (deopt)");
On 2012/09/11 14:36:09, Florian Schneider wrote:
> Why this bailout?

The context register is not (yet) set up properly for inlined functions.

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

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:32: Definition* check) {
On 2012/09/11 14:44:07, srdjan wrote:
> Maybe describe briefly what is 'check' used for in comment.

I think I can get rid of it per Florian's comment below.

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:37: // Abort if the callee has named
parameters.
On 2012/09/11 14:44:07, srdjan wrote:
> why change named -> optionals, as optional is used everywhere below?

Good question.  I changed it back.

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:129: // Replace callees null constant with
callers null constant.
On 2012/09/11 14:44:07, srdjan wrote:
> s/callees/callee's/
> 
> ditto callers

Done.

http://codereview.chromium.org/10916228/diff/1/runtime/vm/flow_graph_inliner....
runtime/vm/flow_graph_inliner.cc:236:
TryInliningInstanceCall(call->instance_call(), call);
On 2012/09/11 14:44:07, srdjan wrote:
> PolymorphicInstanceCall has a with_checks tester. If it returns false, no
check
> should be emitted.

You are right.  And in the case it's needed, there is already a check so no need
to insert one here.

[Florian Schneider, 2012-09-11 16:54:15]

lgtm

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

http://codereview.chromium.org/10916228/diff/4002/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:176: const ICData& ic_data = *call->ic_data();
This can be made even simpler for monomorphic calls:

void VisitPolymorphicInstanceCall(PolymorphicInstanceCallInstr* instr) {
  if (instr->with_checks()) {
    if (FLAG_trace_inlining) OS::Print("Inline aborted: multiple targets");
    return;
  }
  const ICData& ic_data = instr->ic_data();

http://codereview.chromium.org/10916228/diff/4002/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:184: if (ic_data.NumberOfChecks() != 1) {
I think this check is not necesary since we only generate a monomorphic
PolymorphicInstanceCall if the target is unique for all class ids.

http://codereview.chromium.org/10916228/diff/4002/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:191: ic_data.GetCheckAt(0, &class_ids,
&target);
const Function& target = Function::ZoneHandle(ic_data.GetTargetAt(0));

http://codereview.chromium.org/10916228/diff/4002/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:193: if (class_id == kIllegalCid) {
Not sure if those can actually occur in type feedback:

I think you can just
ASSERT (class_id != kIllegalCid);
ASSERT (class_id != kSmiCid);
ASSERT (class_id != kDynamicCid);