More general CHA-based inlining and devirtualization for precompiled code.

More general CHA-based inlining and devirtualization for precompiled code.

Generalize current approach used for implicit getters/setters to all instance calls.

Additionally use propagated type to resolve targets if no concrete cid
is known:

class A {
  void m() { }
  void n() => m();
}
class B extends A { }

Even though the receiver type in n() can be A or B (concrete cid unknown), we can still
call m() directly since it is not overridden.

Also, allow inlining of methods that were not compiled yet in precompiled code.

BUG=
R=rmacnak@google.com, srdjan@google.com

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

[Florian Schneider, 2015-10-21 13:14:46]

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

[Florian Schneider, 2015-10-21 13:29:44]

Description was changed from

==========
More general CHA-based inlining and devirtualization for precompiled code.

Generalize current approach used for implicit getters/setters to all instance
calls.

Additionally use propagated type to resolve targets if no concrete cid
is known:

class A {
  void m() { }
  void n() => m();
}
class B extends A { }

Even though the receiver type in n() can be A or B, we can still call m()
directly since it is not overridden.

Also, allow inlining of methods that were not compiled yet in precompiled code.

BUG=
==========

to

==========
More general CHA-based inlining and devirtualization for precompiled code.

Generalize current approach used for implicit getters/setters to all instance
calls.

Additionally use propagated type to resolve targets if no concrete cid
is known:

class A {
  void m() { }
  void n() => m();
}
class B extends A { }

Even though the receiver type in n() can be A or B (concrete cid unknown), we
can still call m()
directly since it is not overridden.

Also, allow inlining of methods that were not compiled yet in precompiled code.

BUG=
==========

[Florian Schneider, 2015-10-21 13:30:02]

Description was changed from

==========
More general CHA-based inlining and devirtualization for precompiled code.

Generalize current approach used for implicit getters/setters to all instance
calls.

Additionally use propagated type to resolve targets if no concrete cid
is known:

class A {
  void m() { }
  void n() => m();
}
class B extends A { }

Even though the receiver type in n() can be A or B (concrete cid unknown), we
can still call m()
directly since it is not overridden.

Also, allow inlining of methods that were not compiled yet in precompiled code.

BUG=
==========

to

==========
More general CHA-based inlining and devirtualization for precompiled code.

Generalize current approach used for implicit getters/setters to all instance
calls.

Additionally use propagated type to resolve targets if no concrete cid
is known:

class A {
  void m() { }
  void n() => m();
}
class B extends A { }

Even though the receiver type in n() can be A or B (concrete cid unknown), we
can still
call m() directly since it is not overridden.

Also, allow inlining of methods that were not compiled yet in precompiled code.

BUG=
==========

[srdjan, 2015-10-21 17:37:46]

srdjan@google.com changed reviewers:
+ srdjan@google.com

[srdjan, 2015-10-21 17:37:46]

DBC

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/block_schedu...
File runtime/vm/block_scheduler.cc (right):

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/block_schedu...
runtime/vm/block_scheduler.cc:65: return;
When can this happen? In precompilation FLAG_emit_edge_counters is false.

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:256: ((ic_data != NULL) &&
(ic_data->NumberOfUsedChecks() == 0))) {
When do you see ic_data NULL, except for precompilation and there we do not want
to set may_reoptimize to true.

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

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:631: // precompilation mode we don't rely on
type feedback.
This can be a problem for Siva's change where we store type feedback in
snapshots and first compilation is optimized. How about checking the invocation
counter instead of HasCode?

[rmacnak, 2015-10-21 18:00:30]

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:257: may_reoptimize_ = true;
Can we rename this to has_ic_calls_ or, probably better, not flip it when
allow_recompilation_ == false?

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

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:631: // precompilation mode we don't rely on
type feedback.
On 2015/10/21 17:37:46, srdjan wrote:
> This can be a problem for Siva's change where we store type feedback in
> snapshots and first compilation is optimized. How about checking the
invocation
> counter instead of HasCode?

Can this check if the function has an ic data array instead? --collect-code can
drop code but leave the ic data array.

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4406: 1));
1 /* args_tested */

[Florian Schneider, 2015-10-22 13:59:26]

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/block_schedu...
File runtime/vm/block_scheduler.cc (right):

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/block_schedu...
runtime/vm/block_scheduler.cc:65: return;
On 2015/10/21 17:37:46, srdjan wrote:
> When can this happen? In precompilation FLAG_emit_edge_counters is false.

Yes, not needed with precompilation. Left over check when I tried inlining
methods without code with lazy compilation.

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:256: ((ic_data != NULL) &&
(ic_data->NumberOfUsedChecks() == 0))) {
On 2015/10/21 17:37:46, srdjan wrote:
> When do you see ic_data NULL, except for precompilation and there we do not
want
> to set may_reoptimize to true.

Same here. Defensive left over check.

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:257: may_reoptimize_ = true;
On 2015/10/21 18:00:30, rmacnak wrote:
> Can we rename this to has_ic_calls_ or, probably better, not flip it when
> allow_recompilation_ == false?

may_reoptimize_ has no effect with precompilation even now because the
FLAG_optimization_counter_threshold is set to -1.

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

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:631: // precompilation mode we don't rely on
type feedback.
On 2015/10/21 18:00:30, rmacnak wrote:
> On 2015/10/21 17:37:46, srdjan wrote:
> > This can be a problem for Siva's change where we store type feedback in
> > snapshots and first compilation is optimized. How about checking the
> invocation
> > counter instead of HasCode?
> 
> Can this check if the function has an ic data array instead? --collect-code
can
> drop code but leave the ic data array.

I would opt for checking the ic data array, but I did want to 
change inlining heuristics for regular compilation in this change.

Note that this is exactly the same test that we already have with lazy
compilation. I just moved it from Function::CanBeInlined to here so that I can
skip this condition with precompilation.

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4406: 1));
On 2015/10/21 18:00:30, rmacnak wrote:
> 1 /* args_tested */

Done.

[srdjan, 2015-10-22 15:31:26]

LGTM

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

https://codereview.chromium.org/1410733006/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:631: // precompilation mode we don't rely on
type feedback.
On 2015/10/22 13:59:25, Florian Schneider wrote:
> On 2015/10/21 18:00:30, rmacnak wrote:
> > On 2015/10/21 17:37:46, srdjan wrote:
> > > This can be a problem for Siva's change where we store type feedback in
> > > snapshots and first compilation is optimized. How about checking the
> > invocation
> > > counter instead of HasCode?
> > 
> > Can this check if the function has an ic data array instead? --collect-code
> can
> > drop code but leave the ic data array.
> 
> I would opt for checking the ic data array, but I did want to 
> change inlining heuristics for regular compilation in this change.
> 
> Note that this is exactly the same test that we already have with lazy
> compilation. I just moved it from Function::CanBeInlined to here so that I can
> skip this condition with precompilation.
> 

OK, to replicate current behavior. CC-ing Siva to know what to fix to enable
inlining in snapshots with instant optimization

[rmacnak, 2015-10-22 16:32:54]

lgtm

[Florian Schneider, 2015-10-23 11:34:37]

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