Allocate instance closures similarly to regular closures, i.e. without a

Allocate instance closures similarly to regular closures, i.e. without a
specific stub and runtime call.

R=fschneider@google.com

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

[Florian Schneider, 2014-02-24 18:50:23]

https://codereview.chromium.org/178233003/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/178233003/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2389: node->receiver()->Visit(&for_receiver);
Is there a dependency on the evaluation order of the type arguments and the
receiver? You moved the evaluation of the receiver across the evaluation of the
type arguments (BuildInstantiatorTypeArguments). I'm not sure if that is
allowed.

I think if you want to copy the same evaluation order as in the old code, you'd
have to evaluate the receiver outside, and store it in a temporary local scope.

In this case it may make sense to generalize TempLocalScope to allow allocating
more than one temp at once.

https://codereview.chromium.org/178233003/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2398: new
StoreContextInstr(Bind(ExitTempLocalScope(tmp_var))));
The StoreContext does not appear in the original code. I think you can just drop
the allocated context by using

Do(ExitTempLocalScope(context_var));

https://codereview.chromium.org/178233003/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2402: Do(new
StoreInstanceFieldInstr(context_field,
Try move this store into the inner TempLocalScope: allocated_context is already
consumed and can't be used twice. You also have to rename tmp_var into something
else to avoid a name conflict. For example closure_tmp_var and context_temp_var.

[regis, 2014-02-24 19:01:30]

https://codereview.chromium.org/178233003/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/178233003/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2389: node->receiver()->Visit(&for_receiver);
On 2014/02/24 18:50:23, Florian Schneider wrote:
> Is there a dependency on the evaluation order of the type arguments and the
> receiver? You moved the evaluation of the receiver across the evaluation of
the
> type arguments (BuildInstantiatorTypeArguments). I'm not sure if that is
> allowed.
> 
> I think if you want to copy the same evaluation order as in the old code,
you'd
> have to evaluate the receiver outside, and store it in a temporary local
scope.
> 
> In this case it may make sense to generalize TempLocalScope to allow
allocating
> more than one temp at once.

I do not think that the evaluation of the type arguments can have any side
effects, but I will double check.

https://codereview.chromium.org/178233003/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2398: new
StoreContextInstr(Bind(ExitTempLocalScope(tmp_var))));
On 2014/02/24 18:50:23, Florian Schneider wrote:
> The StoreContext does not appear in the original code. I think you can just
drop
> the allocated context by using
> 
> Do(ExitTempLocalScope(context_var));

No idea why I have this StoreContextInstr here.

https://codereview.chromium.org/178233003/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2402: Do(new
StoreInstanceFieldInstr(context_field,
On 2014/02/24 18:50:23, Florian Schneider wrote:
> Try move this store into the inner TempLocalScope: allocated_context is
already
> consumed and can't be used twice. You also have to rename tmp_var into
something
> else to avoid a name conflict. For example closure_tmp_var and
context_temp_var.

Yes, exactly. I already renamed these as you suggested after sending my email.
This was really just a rough draft. Let me work in this some more. Thanks!

[regis, 2014-02-24 22:09:03]

Next draft uploaded. The corresponding flow graph is below.
I do not see anything obviously wrong, but it is still not working.

==== dart:async__Future@0x2900bd4a_get__completeError@0x2900bd4a
B0[graph]:2
B1[target]:0
    CheckStackOverflow:4()
    t0 <- AllocateObject:6(Library:'dart:async' Class:
_Future@0x2900bd4a<T>(dynamic, dynamic, [StackTrace]) => void) T{not-null,
_Future@0x2900bd4a<T>(dynamic, dynamic, [StackTrace]) => void, Type: class
'_Future@0x2900bd4a<T>(dynamic, dynamic, [StackTrace]) => void'}
    PushTemp:8(t0)
    t1 <- LoadLocal:10(:tmp_local-3)
    t2 <- Constant:12(#Function '_completeError@0x2900bd4a':.)
    StoreInstanceField:14(:function {8}, t1, t2)
    t1 <- AllocateContext:16(1) T{not-null, Context, Type: class 'dynamic'}
    PushTemp:18(t1)
    t2 <- LoadLocal:20(:tmp_local-4)
    t3 <- LoadLocal:22(this)
    StoreVMField:24(t2, 16, t3)
    t2 <- LoadLocal:26(:tmp_local-3)
    t3 <- LoadLocal:28(:tmp_local-4)
    StoreInstanceField:30(:context {12}, t2, t3)
    t2 <- LoadLocal:32(:tmp_local-4)
    DropTemps:34(t2)
    t1 <- LoadLocal:36(:tmp_local-3)
    t0 <- DropTemps:38(t1)
    Return:40(t0)
*** END CFG

[Florian Schneider, 2014-02-25 09:21:15]

https://codereview.chromium.org/178233003/diff/50001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/178233003/diff/50001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2389: Value* context_tmp_val = Bind(new
LoadLocalInstr(*context_tmp_var));
In StoreVMFieldInstr the order of arguments is reversed relative to
StoreInstanceField. It is an artifact of how we build stores to captured locals.
(and should probably be changed, since it is annoying to get right)

I think the bug is fixed by reordering evaluation of the input operands here:

// First operand (receiver).
ValueGraphVisitor for_receiver(owner());
node->receiver()->Visit(&for_receiver);
Append(for_receiver);
Value* receiver = for_receiver.value();

// Second operand (context)
Value* context_tmp_val = Bind(new LoadLocalInstr(*context_tmp_var));

[regis, 2014-02-25 17:35:12]

On 2014/02/25 09:21:15, Florian Schneider wrote:
>
https://codereview.chromium.org/178233003/diff/50001/runtime/vm/flow_graph_bu...
> File runtime/vm/flow_graph_builder.cc (right):
> 
>
https://codereview.chromium.org/178233003/diff/50001/runtime/vm/flow_graph_bu...
> runtime/vm/flow_graph_builder.cc:2389: Value* context_tmp_val = Bind(new
> LoadLocalInstr(*context_tmp_var));
> In StoreVMFieldInstr the order of arguments is reversed relative to
> StoreInstanceField. It is an artifact of how we build stores to captured
locals.
> (and should probably be changed, since it is annoying to get right)
> 
> I think the bug is fixed by reordering evaluation of the input operands here:
> 
> // First operand (receiver).
> ValueGraphVisitor for_receiver(owner());
> node->receiver()->Visit(&for_receiver);
> Append(for_receiver);
> Value* receiver = for_receiver.value();
> 
> // Second operand (context)
> Value* context_tmp_val = Bind(new LoadLocalInstr(*context_tmp_var));

Fantastic! That was it. Thank you.
Let me merge the regular closure case with the instance closure case and remove
all the obsolete code, and I'll resend for review.

[regis, 2014-02-25 23:40:02]

Now ready for review.

Thanks,
Regis

[Ivan Posva, 2014-02-26 05:16:02]

-Ivan

https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2287: const Field& function_field =
In a separate CL we should make these singletons in the isolate or even better
in the VM isolate.

[Florian Schneider, 2014-02-26 11:46:06]

LGTM.

https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2287: const Field& function_field =
On 2014/02/26 05:16:02, Ivan Posva wrote:
> In a separate CL we should make these singletons in the isolate or even better
> in the VM isolate.

I would just allocate one instance per compilation and attach them to the flow
graph for example.

Why make them permanent in the VM isolate? Since they are only attached to the
IL during compilation and used for load elimination and allocation sinking, they
can be GCed after compilation finished.

[regis, 2014-02-26 18:38:14]

On 2014/02/26 11:46:06, Florian Schneider wrote:
> LGTM.
> 
>
https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
> File runtime/vm/flow_graph_builder.cc (right):
> 
>
https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
> runtime/vm/flow_graph_builder.cc:2287: const Field& function_field =
> On 2014/02/26 05:16:02, Ivan Posva wrote:
> > In a separate CL we should make these singletons in the isolate or even
better
> > in the VM isolate.
> 
> I would just allocate one instance per compilation and attach them to the flow
> graph for example.
> 
> Why make them permanent in the VM isolate? Since they are only attached to the
> IL during compilation and used for load elimination and allocation sinking,
they
> can be GCed after compilation finished.

Thanks!

Florian, let me know if you want to implement what you suggest with attaching
the fields. Otherwise, I'll have a look later.

[regis, 2014-02-26 18:38:58]

Committed patchset #4 manually as r33074 (presubmit successful).

[Ivan Posva, 2014-02-27 18:12:33]

Filed http://dartbug.com/17160

-Ivan

https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:2287: const Field& function_field =
On 2014/02/26 11:46:07, Florian Schneider wrote:
> On 2014/02/26 05:16:02, Ivan Posva wrote:
> > In a separate CL we should make these singletons in the isolate or even
better
> > in the VM isolate.
> 
> I would just allocate one instance per compilation and attach them to the flow
> graph for example.
> 
> Why make them permanent in the VM isolate? Since they are only attached to the
> IL during compilation and used for load elimination and allocation sinking,
they
> can be GCed after compilation finished.

Florian, the code below is technically wrong. The field it is referencing are
not part of the class it is using to define them alloc-cls() is the signature
class, but the fields are really defined in the super class. So I still say
these two fields could be pre-allocated at startup and added to the object store
without having to ever allocate them again.

[Florian Schneider, 2014-02-28 10:46:51]

On 2014/02/27 18:12:33, Ivan Posva wrote:
> Filed http://dartbug.com/17160
> 
> -Ivan
> 
>
https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
> File runtime/vm/flow_graph_builder.cc (right):
> 
>
https://codereview.chromium.org/178233003/diff/70001/runtime/vm/flow_graph_bu...
> runtime/vm/flow_graph_builder.cc:2287: const Field& function_field =
> On 2014/02/26 11:46:07, Florian Schneider wrote:
> > On 2014/02/26 05:16:02, Ivan Posva wrote:
> > > In a separate CL we should make these singletons in the isolate or even
> better
> > > in the VM isolate.
> > 
> > I would just allocate one instance per compilation and attach them to the
flow
> > graph for example.
> > 
> > Why make them permanent in the VM isolate? Since they are only attached to
the
> > IL during compilation and used for load elimination and allocation sinking,
> they
> > can be GCed after compilation finished.
> 
> Florian, the code below is technically wrong. The field it is referencing are
> not part of the class it is using to define them alloc-cls() is the signature
> class, but the fields are really defined in the super class. So I still say
> these two fields could be pre-allocated at startup and added to the object
store
> without having to ever allocate them again.

How is it technically wrong? I think it would be _ok_ to add them to
FunctionImpl, but it is not needed for correctness of code generation - If you
want those fields to detected by reflection, then they should be real fields
linked to the FunctionImpl class.

Otherwise, if that is not needed they really just serve for memory
disambiguation in load elimination, so one instance per compilation unit would
be fine. For the compiler, they are just a wrapper for an offset and some
properties (like immutability for finals, etc.) Instead we could actually just
do LoadField from the corresponding offset without ever allocating a field.