More inlining flags and tuned heuristics.

More inlining flags and tuned heuristics.

The heuristics should be examined further, but these maintain about the same
runtime for the benchmarks while decreasing compile time for dart2js.

R=kmillikin@google.com

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

[Kevin Millikin (Google), 2012-10-25 14:44:08]

The loop nesting depth for break and continue needs to be set when we add
connect them to the loop (by adding the last incoming edge).

We can't use the nesting depth in the body because we can break or continue to a
label.

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

http://codereview.chromium.org/11269040/diff/2001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:26: "Inline function calls up to threshold
nesting depth");
Inline ==> Always inline

http://codereview.chromium.org/11269040/diff/2001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:28: "Inline function calls that have up to
threshold instruction");
instruction ==> instructions

http://codereview.chromium.org/11269040/diff/2001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:32: "Inline function calls containing up to
threshold call sites.");
"Always inline functions containing threshold or fewer calls."

http://codereview.chromium.org/11269040/diff/2001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:262: return ((size <=
FLAG_inlining_size_threshold) ||
This might be easier to read and modify if it were written:

if (size <= FLAG_inlining_size_threshold) return true;

if (call_sites <= FLAG_inlining_callee_call_sites_threshold) return true;

// etc.

return false;

[srdjan, 2012-10-25 20:38:50]

DBC.

Tracking of loop depth is pretty pervasive. Does it bring anything? If you
disable the loop heuristics, does it change anything? How much is the speed of
dart2js improved?

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1230: join->set_loop_depth(loop_depth());
It looks weird that an instruction changes its loop depth.

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1272: BlockEntryInstr::kInvalidLoopDepth);
Why not set it to loop_depth() + 1 ?

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

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.h:291: intptr_t loop_depth_;
const ?

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

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:38: "and up to the increased threshold on
instructions");
Indent  4 characters (continuation of a 'statement').

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:143: // Helper to collect information about a
callee graph.
about inlineable callee graphs

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:146: explicit GraphInfoCollector()
remove explicit

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:259: intptr_t size,
Size of what? Please name better

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:260: intptr_t call_sites,
num_call_sites?

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:359: }
Could you use a modified inlining_size_threshold value to copmpare it against
the code size of the function (class Code) instead of bailing out after the
function has been parsed and the graph has been built?

[Kevin Millikin (Google), 2012-10-26 12:07:22]

I agree that the loop depth is invasive.  Many calls to construct a visitor look
like:

... for_effect(owner(), temp_index(), loop_depth());

We could define a second constructor that took a 'parent' visitor and replace
those with:

... for_effect(this);

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:198: loop_depth());
This join is the entry to the test.  It should be at loop_depth() + 1.

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1230: join->set_loop_depth(loop_depth());
On 2012/10/25 20:38:50, srdjan wrote:
> It looks weird that an instruction changes its loop depth.

The loop depth was invalid before.  We do not know the loop depth of the break
block at the break statements --- because we can break out of a loop.

It's only when we see this incoming edge that we know the loop depth.

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

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:359: }
On 2012/10/25 20:38:50, srdjan wrote:
> Could you use a modified inlining_size_threshold value to copmpare it against
> the code size of the function (class Code) instead of bailing out after the
> function has been parsed and the graph has been built?

We could definitely do that.  In fact, we could cache things like number of
instructions, number of calls, etc. in the function --- as long as we can
compute them from the unoptimized compilation.

[zerny-google, 2012-10-29 16:44:25]

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
File runtime/vm/flow_graph_builder.cc (right):

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:198: loop_depth());
On 2012/10/26 12:07:22, kmillikin wrote:
> This join is the entry to the test.  It should be at loop_depth() + 1.

Done.

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.cc:1272: BlockEntryInstr::kInvalidLoopDepth);
On 2012/10/25 20:38:50, srdjan wrote:
> Why not set it to loop_depth() + 1 ?

I set it to invalid so it is correctly set just once after the if. I can change
that and the if-then to an if-then-else.

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

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_build...
runtime/vm/flow_graph_builder.h:291: intptr_t loop_depth_;
On 2012/10/25 20:38:50, srdjan wrote:
> const ?

Done.

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

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:38: "and up to the increased threshold on
instructions");
On 2012/10/25 20:38:50, srdjan wrote:
> Indent  4 characters (continuation of a 'statement').

Done.

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:143: // Helper to collect information about a
callee graph.
On 2012/10/25 20:38:50, srdjan wrote:
> about inlineable callee graphs

Done.

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:146: explicit GraphInfoCollector()
On 2012/10/25 20:38:50, srdjan wrote:
> remove explicit

Done.

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:259: intptr_t size,
On 2012/10/25 20:38:50, srdjan wrote:
> Size of what? Please name better

s/size/instr_count

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:260: intptr_t call_sites,
On 2012/10/25 20:38:50, srdjan wrote:
> num_call_sites?

s/call_sites/call_site_count

http://codereview.chromium.org/11269040/diff/6001/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:359: }
On 2012/10/26 12:07:22, kmillikin wrote:
> On 2012/10/25 20:38:50, srdjan wrote:
> > Could you use a modified inlining_size_threshold value to copmpare it
against
> > the code size of the function (class Code) instead of bailing out after the
> > function has been parsed and the graph has been built?
> 
> We could definitely do that.  In fact, we could cache things like number of
> instructions, number of calls, etc. in the function --- as long as we can
> compute them from the unoptimized compilation.

The is_inlinable bit approximated this (line 479), but with this CL I changed
the behavior so the bit is set to low only if the size is above all the
thresholds. This way, we can use different heuristics at different call sites.
Of course, this means a function between size_threshold and
in_loop_size_threshold will be parsed and built repeatedly even if it will only
be inlined when in a loop context.

[Kevin Millikin (Google), 2012-11-02 11:30:25]

LGTM.