VM: Speculative inlining in precompiled code.

VM: Speculative inlining in precompiled code.

Enable inlining of certain smi- and array-operations based on propagated
types and range analysis:

If bounds checks and class checks can not be eliminated, bail out of the
current optimization and retry without speculative inlining using the same
mechanism as we use for far jumps on MIPS.

Allow more speculative inlining attempts with up to n deopt ids black-listed.
For now set n=1 since precompilation time will be proportional to n.


BUG=
R=srdjan@google.com

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

[Florian Schneider, 2015-11-17 12:39:06]

Description was changed from

==========
VM: Speculative inlining in precompiled code.

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

to

==========
VM: Speculative inlining in precompiled code.

Enable inlining of certain smi- and array-operations based on propagated
types and range analysis:

If bounds checks and class checks can not be eliminated, bail out of the
current optimization and retry without speculative inlining using the same
mechanism as we use for far jumps on MIPS.

Allow more speculative inlining attempts with up to n deopt ids black-listed.
For now set n=1 since precompilation time will be proportional to n.


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

[Florian Schneider, 2015-11-17 12:39:24]

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

[Florian Schneider, 2015-11-17 12:39:25]

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:513: optimizer.ApplyClassIds();
Adding an early pass of specializing based on guessed-cids early, so that later
passes of ApplyICData/ApplyClassIds can benefit from it.

[srdjan, 2015-11-17 17:35:24]

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:422: intptr_t val = setjmp(*jump.Set());
Add Comment that val may contain deopt_id of the failed inlining attempt.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:422: intptr_t val = setjmp(*jump.Set());
const intptr_t val =

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:890: }
The loop above should be tun in DEBUG mode only. Add comment: Do not repeatedly
abort speculative inlining at the same call.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:893: if (inlining_black_list.length() >= kMaxAttempts) {
Use flag instead of kMaxAttempt.

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:922: THR_Print("Retrying compilation %s,
suppressing deopt_id:%" Pd "\n",
suppressing inlining of call deopt_id: ...

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:923: parsed_function_.function().ToCString(),
deopt_id);
Maybe ToQualifiedCString gives a little bit more information.

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4205: bool
FlowGraphOptimizer::IsBlackListed(intptr_t deopt_id) {
s/deopt_id/call_deopt_id/ makes it clearer IMO.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4205: bool
FlowGraphOptimizer::IsBlackListed(intptr_t deopt_id) {
s/IsBlackListed/ IsBlackListedForInlining/

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4242: unary_checks.NumberOfChecks() > 0) {
Add parentheses

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4249: if (op_kind == Token::kEQ &&
TryReplaceWithEqualityOp(instr, op_kind)) {
Add parentheses

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5215: Compiler::always_optimize()) {
Add comment why not do it for precompilation.

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.h:19: explicit FlowGraphOptimizer(FlowGraph*
flow_graph,
Please add comment that use_speculative_inlining and black lists are only used
when always optimizing and speculative inlining is enabled ... or similar.
s/black_list/inlining_black_list/ add comment that it contains deopt-ids of
calls that may not be inlined.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.h:25: inlining_black_list_(black_list) { }
ASSERT(!use_speculative_inlining || (black_list != NULL)); 

Serves as documentation as well./

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.h:275: bool use_speculative_inlining_;
const

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_r...
File runtime/vm/flow_graph_range_analysis.cc (right):

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_r...
runtime/vm/flow_graph_range_analysis.cc:1534: !Compiler::always_optimize();
Please add a comment why we should not try_generalization for precompilation.

[Florian Schneider, 2015-11-17 19:38:57]

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:422: intptr_t val = setjmp(*jump.Set());
On 2015/11/17 17:35:23, srdjan wrote:
> const intptr_t val =

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:422: intptr_t val = setjmp(*jump.Set());
On 2015/11/17 17:35:23, srdjan wrote:
> Add Comment that val may contain deopt_id of the failed inlining attempt.

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:890: }
On 2015/11/17 17:35:23, srdjan wrote:
> The loop above should be tun in DEBUG mode only. Add comment: Do not
repeatedly
> abort speculative inlining at the same call.

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:893: if (inlining_black_list.length() >= kMaxAttempts) {
On 2015/11/17 17:35:23, srdjan wrote:
> Use flag instead of kMaxAttempt.

Done.

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:922: THR_Print("Retrying compilation %s,
suppressing deopt_id:%" Pd "\n",
On 2015/11/17 17:35:23, srdjan wrote:
> suppressing inlining of call deopt_id: ...

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:923: parsed_function_.function().ToCString(),
deopt_id);
On 2015/11/17 17:35:23, srdjan wrote:
> Maybe ToQualifiedCString gives a little bit more information. 

Done.

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4205: bool
FlowGraphOptimizer::IsBlackListed(intptr_t deopt_id) {
On 2015/11/17 17:35:23, srdjan wrote:
> s/deopt_id/call_deopt_id/ makes it clearer IMO.

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4205: bool
FlowGraphOptimizer::IsBlackListed(intptr_t deopt_id) {
On 2015/11/17 17:35:23, srdjan wrote:
> s/IsBlackListed/ IsBlackListedForInlining/

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4242: unary_checks.NumberOfChecks() > 0) {
On 2015/11/17 17:35:23, srdjan wrote:
> Add parentheses

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:4249: if (op_kind == Token::kEQ &&
TryReplaceWithEqualityOp(instr, op_kind)) {
On 2015/11/17 17:35:23, srdjan wrote:
> Add parentheses

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:5215: Compiler::always_optimize()) {
On 2015/11/17 17:35:23, srdjan wrote:
> Add comment why not do it for precompilation.

Done.

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

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.h:25: inlining_black_list_(black_list) { }
On 2015/11/17 17:35:23, srdjan wrote:
> ASSERT(!use_speculative_inlining || (black_list != NULL)); 
> 
> Serves as documentation as well./

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.h:275: bool use_speculative_inlining_;
On 2015/11/17 17:35:23, srdjan wrote:
> const

Done.

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_r...
File runtime/vm/flow_graph_range_analysis.cc (right):

https://codereview.chromium.org/1423063005/diff/20001/runtime/vm/flow_graph_r...
runtime/vm/flow_graph_range_analysis.cc:1534: !Compiler::always_optimize();
On 2015/11/17 17:35:24, srdjan wrote:
> Please add a comment why we should not try_generalization for precompilation.

Done.

[srdjan, 2015-11-17 20:39:19]

lgtm

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

https://codereview.chromium.org/1423063005/diff/40001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:891: #ifdef DEBUG
#if defined(DEBUG)  ... probably both equivalent, but we try to use #if
defined(..) consistently.

https://codereview.chromium.org/1423063005/diff/40001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.h (right):

https://codereview.chromium.org/1423063005/diff/40001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.h:21: GrowableArray<intptr_t>* black_list =
NULL)
s/black_list/inlining_black_list/

[Florian Schneider, 2015-11-18 13:56:11]

Committed patchset #4 (id:60001) manually as
dce89b9b5b71a417b9f8779af0ea1b0d88dc0d07 (presubmit successful).

[Florian Schneider, 2015-11-18 14:01:05]

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

https://codereview.chromium.org/1423063005/diff/40001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:891: #ifdef DEBUG
On 2015/11/17 20:39:19, srdjan wrote:
> #if defined(DEBUG)  ... probably both equivalent, but we try to use #if
> defined(..) consistently.

Yes, they are equivalent.

https://codereview.chromium.org/1423063005/diff/40001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.h (right):

https://codereview.chromium.org/1423063005/diff/40001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.h:21: GrowableArray<intptr_t>* black_list =
NULL)
On 2015/11/17 20:39:19, srdjan wrote:
> s/black_list/inlining_black_list/

Done.