Support allocating and calling closures in the new non-optimizing compiler.

Support allocating and calling closures in the new non-optimizing compiler.

As part of this change, variable allocation is split out from the backend
code generator.

R=srdjan@google.com
BUG=
TEST=

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

[Kevin Millikin (Google), 2012-03-12 14:06:36]

It turns out to be hard and error-prone, to try to make the variable allocation
code in scopes.cc so it can be run more than once.

I split it out.  The bits of state that need to be persisted are put on the
ParsedFunction (because they are per-function, not per-scope).  If there's a
better place for it, let me know.

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

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1491: // Allocate variables before constructing
the graph.
This comment is out of date, I'm deleting it.

[srdjan, 2012-03-12 18:35:23]

LGTM with one question/comment

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/code_generat...
File runtime/vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/code_generat...
runtime/vm/code_generator_ia32.cc:343: // - allocate local variables on stack.
Remove this step. Maybe write as a comment that the local variables have already
been allocated.

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/code_generat...
runtime/vm/code_generator_ia32.cc:741: (function.context_scope() ==
ContextScope::null())) {
Why the changed condition? Previously we did not go to else clause if
IsNonImplicitClosureFunction was true, now we do.

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

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:813: if
(function.IsNonImplicitClosureFunction()) {
This condition does not match what is in the (changed) code_generator_xxx.cc

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:814: const int context_level = 0;  // Only
because we don't handle nesting yet.
Dont you need to check that state()->context_level() == 0?

[srdjan, 2012-03-12 19:56:17]

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

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1495: }
Why do we bailout here and not in the FlowGraphCompiler.

[srdjan, 2012-03-12 22:55:49]

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:147: local_count_ = first_local_index_ -
next_free_frame_index;
+ copied_parameter_count_;

[Kevin Millikin (Google), 2012-03-13 08:59:49]

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/code_generat...
File runtime/vm/code_generator_ia32.cc (right):

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/code_generat...
runtime/vm/code_generator_ia32.cc:343: // - allocate local variables on stack.
On 2012/03/12 18:35:23, srdjan wrote:
> Remove this step. Maybe write as a comment that the local variables have
already
> been allocated.

OK, removed.  I thought it was referring to reserving space for them on stack.

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/code_generat...
runtime/vm/code_generator_ia32.cc:741: (function.context_scope() ==
ContextScope::null())) {
On 2012/03/12 18:35:23, srdjan wrote:
> Why the changed condition? Previously we did not go to else clause if
> IsNonImplicitClosureFunction was true, now we do.

Now we can have both IsNonImplicitClosureFunction() and context_scope() !=
ContextScope::null(), because it was set by the new backend which later bailed
out.

I suppose here we could throw it away and reset it, but I thought it better to
assume it was correct and use it.

The else only contains an if now, and I think IsNonImplicitClosureFunction() and
IsImplicitInstanceClosureFunction() are mutually exclusive, so the meaning
should be the same as before.

In any case, I will restructure it to be more clear.  What do you think of:

if (...IsNonImplicitClosureFunction()) {
  if (...context_scope() == ContextScope::null()) {
     ...
  }
} else if (...IsImplicitClosureFunction()) {
  ...
}

?

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

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:814: const int context_level = 0;  // Only
because we don't handle nesting yet.
On 2012/03/12 18:35:23, srdjan wrote:
> Dont you need to check that state()->context_level() == 0?

Eventually, or handle non-zero.  There is no context level tracking in this
compiler, yet.

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:1495: }
On 2012/03/12 19:56:17, srdjan wrote:
> Why do we bailout here and not in the FlowGraphCompiler.

I moved the bailout because I initially moved the variable allocation here
(needed for graph translation of closures and context variables correctly, in
the very back end is too late); and because the graph translation assumes (right
now) that we have already bailed out so it doesn't set an invalid context scope
on a function, that we could potentially observe if we bail out of this
compiler. 

When I realized that there was no good way to safely repeat the variable
allocation (it mutates variables, which are in the AST shared by the other
compiler, and so a pain in the butt to clear), I moved it out of the compilers.

It still seems like a good idea to bail out earlier for global properties like
this, rather than translate the graph and bail out much later.  It is safer and
easier to prototype this way---the graph translation can assume that we have
bailed out already in many cases.

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/9664062/diff/1/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:147: local_count_ = first_local_index_ -
next_free_frame_index;
On 2012/03/12 22:55:49, srdjan wrote:
> + copied_parameter_count_;

Consistent terminology will help.

Maybe the name is too generic, but this is intended to *not* include the copied
parameter count (which is actually a property independent of variable
allocation, and) which is already counted in the field copied_parameter_count_. 
I want the name "local" to mean the same thing in "first_local_index_" and
"local_count_".

I don't know what would be a better name.  stack_local_count_ is more accurate,
but still not obvious.

This field is used in two ways: once to decide how much space to reserve in the
frame (where we need to include copied parameter count, and the code currently
adds the two quantities at the use site), and once when we need to initialize
only the local variables (ignoring copied parameters and starting from
first_local_index_).

In any case, we either need to add copied parameters (to get total) at one of
the use sites, or else subtract (to get just local variables) at the other use
site.  Or add another, redundant, field (or accessor), but I really don't like
that.