Nested deoptimization environments.

Nested deoptimization environments.

In preparation for inlining with deoptimization support.

Committed: http://code.google.com/p/dart/source/detail?r=12190

[zerny-google, 2012-09-07 11:28:44]

Here is a separate CL restructuring deopt environments prior to inlining.

[Kevin Millikin (Google), 2012-09-07 12:51:52]

Maybe it doesn't matter because we don't have much inlining enabled --- but I
think there are a few places where we need to handle the entire deopt
environment, not just the local part.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
runtime/vm/flow_graph_allocator.cc:149: for (intptr_t i = 0; i <
current->env()->Length(); ++i) {
This seems wrong.  Why are values in outer environments not used here?

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
runtime/vm/flow_graph_allocator.cc:749: if (env->Length() == 0) return;
This also seems wrong.  What if there are values in outer environments?

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
runtime/vm/flow_graph_compiler.cc:64: for (intptr_t i = 0; i <
deoptimization_env_->Length(); i++) {
This also seems wrong.  There can be pushed arguments in outer environments. 
Consider:

f(e0, e1, g())

in the body of inlined function g, where f is not inlined.  The pushed values of
e0 and e1 will be found in an outer environment.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:1618: Environment* env = new Environment(
All other things being equal, I tend to try to break the line at the lowest
precedence operator.  = is lower than (), so:

Environment* env =
    new Environment(definitions.length(),
                    fixed_parameter_count,
                    -1,
                    (outer == NULL) ? NULL : outer->Copy());

It uses the same amount of space and avoids the unsightly hanging '(' and
offside indentation of the following line.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:1621: -1,
Isolate::kNoDeoptId,

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:1631: Environment* copy = new Environment(
Same comment about indentation.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.h:3300: class EnvironmentIterator : public
ValueObject {
It's a bit confusing that we can't see at the use sites whether this is
iterating the whole environment or the local one.

In Crankshaft we used the terms "deep" and "shallow" for the two distinct kinds
of iterators.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.h:3313: return env_ == NULL || ix_ ==
env_->values_.length();
The second part of this can never occur?

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.h:3332: void ReplaceValue(Value* value) {
Probably should be ReplaceCurrentValue, also for locations below.

[zerny-google, 2012-09-07 13:51:37]

Your comments should be addressed. I'll let you dcommit this since I am gone
until next Friday.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_allocator.cc (right):

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
runtime/vm/flow_graph_allocator.cc:149: for (intptr_t i = 0; i <
current->env()->Length(); ++i) {
On 2012/09/07 12:51:52, kmillikin wrote:
> This seems wrong.  Why are values in outer environments not used here?

I think this one is safe, but I have tried only to iterate the entire env. in
safe locations. I'll update to iterators/recursive procedures in the
inline-deopt CL.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
runtime/vm/flow_graph_allocator.cc:749: if (env->Length() == 0) return;
On 2012/09/07 12:51:52, kmillikin wrote:
> This also seems wrong.  What if there are values in outer environments?

This needs to be defined recursively on the nested envs, since locations are
attached at each individual env.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
File runtime/vm/flow_graph_compiler.cc (right):

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/flow_gra...
runtime/vm/flow_graph_compiler.cc:64: for (intptr_t i = 0; i <
deoptimization_env_->Length(); i++) {
On 2012/09/07 12:51:52, kmillikin wrote:
> This also seems wrong.  There can be pushed arguments in outer environments. 
> Consider:
> 
> f(e0, e1, g())
> 
> in the body of inlined function g, where f is not inlined.  The pushed values
of
> e0 and e1 will be found in an outer environment.

Yes. And this needs to be recursive (with a retn-addr before and the pc-marker
and fp after). Will change in the inline-deopt CL.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:1618: Environment* env = new Environment(
On 2012/09/07 12:51:52, kmillikin wrote:
> All other things being equal, I tend to try to break the line at the lowest
> precedence operator.  = is lower than (), so:
> 
> Environment* env =
>     new Environment(definitions.length(),
>                     fixed_parameter_count,
>                     -1,
>                     (outer == NULL) ? NULL : outer->Copy());
> 
> It uses the same amount of space and avoids the unsightly hanging '(' and
> offside indentation of the following line.

Done.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:1621: -1,
On 2012/09/07 12:51:52, kmillikin wrote:
> Isolate::kNoDeoptId,

Done.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:1631: Environment* copy = new Environment(
On 2012/09/07 12:51:52, kmillikin wrote:
> Same comment about indentation.

Done.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.h:3300: class EnvironmentIterator : public
ValueObject {
On 2012/09/07 12:51:52, kmillikin wrote:
> It's a bit confusing that we can't see at the use sites whether this is
> iterating the whole environment or the local one.
> 
> In Crankshaft we used the terms "deep" and "shallow" for the two distinct
kinds
> of iterators.

I will defer this choice and change to you.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.h:3313: return env_ == NULL || ix_ ==
env_->values_.length();
On 2012/09/07 12:51:52, kmillikin wrote:
> The second part of this can never occur?

It can for an environment of length zero.

https://chromiumcodereview.appspot.com/10928048/diff/2001/runtime/vm/intermed...
runtime/vm/intermediate_language.h:3332: void ReplaceValue(Value* value) {
On 2012/09/07 12:51:52, kmillikin wrote:
> Probably should be ReplaceCurrentValue, also for locations below.

Done.