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

runtime/vm/flow_graph_builder.cc

Index: runtime/vm/flow_graph_builder.cc
===================================================================
--- runtime/vm/flow_graph_builder.cc	(revision 32974)
+++ runtime/vm/flow_graph_builder.cc	(working copy)
@@ -2321,21 +2321,16 @@
       ReturnDefinition(ExitTempLocalScope(tmp_var));
     }
   } else {
+    // TODO(regis): Merge this branch in the above one.
     ASSERT(function.IsImplicitInstanceClosureFunction());
-    ValueGraphVisitor for_receiver(owner());
-    node->receiver()->Visit(&for_receiver);
-    Append(for_receiver);
-    Value* receiver = for_receiver.value();
 
-    PushArgumentInstr* push_receiver = PushArgument(receiver);
     ZoneGrowableArray<PushArgumentInstr*>* arguments =
-        new ZoneGrowableArray<PushArgumentInstr*>(2);
-    arguments->Add(push_receiver);
+        new ZoneGrowableArray<PushArgumentInstr*>(2);  // 1? here and above too?
     ASSERT(function.context_scope() != ContextScope::null());
 
     // The function type of a closure may have type arguments. In that case,
     // pass the type arguments of the instantiator. Otherwise, pass null object.
-    const Class& cls = Class::Handle(function.signature_class());
+    const Class& cls = Class::ZoneHandle(function.signature_class());
     ASSERT(!cls.IsNull());
     const bool requires_type_arguments = cls.NumTypeArguments() > 0;
     Value* type_arguments = NULL;
@@ -2345,13 +2340,71 @@
       type_arguments = BuildInstantiatorTypeArguments(node->token_pos(),
                                                       instantiator_class,
                                                       NULL);
-    } else {
-      type_arguments = BuildNullValue();
+      arguments->Add(PushArgument(type_arguments));
     }
-    PushArgumentInstr* push_type_arguments = PushArgument(type_arguments);
-    arguments->Add(push_type_arguments);
-    ReturnDefinition(
-        new CreateClosureInstr(node->function(), arguments, node->token_pos()));
+    AllocateObjectInstr* alloc = new AllocateObjectInstr(node->token_pos(),
+                                                         cls,
+                                                         arguments);
+    alloc->set_closure_function(function);
+
+    // Create fake fields for function and context. Only the context field is
+    // stored at the allocation to be used later when inlining a closure call.
+    const Field& function_field =
+        Field::ZoneHandle(
+            Field::New(Symbols::ClosureFunctionField(),
+            false,  // !static
+            false,  // !final
+            false,  // !const
+            alloc->cls(),
+            0));  // No token position.
+    function_field.SetOffset(Closure::function_offset());
+    const Field& context_field =
+        Field::ZoneHandle(Field::New(
+            Symbols::ClosureContextField(),
+            false,  // !static
+            false,  // !final
+            false,  // !const
+            alloc->cls(),
+            0));  // No token position.
+    context_field.SetOffset(Closure::context_offset());
+    alloc->set_context_field(context_field);
+
+    Value* closure_val = Bind(alloc);
+    { LocalVariable* tmp_var = EnterTempLocalScope(closure_val);
+      // Store function.
+      Value* tmp_val = Bind(new LoadLocalInstr(*tmp_var));
+      Value* func_val =
+          Bind(new ConstantInstr(Function::ZoneHandle(function.raw())));
+      Do(new StoreInstanceFieldInstr(function_field,
+                                     tmp_val,
+                                     func_val,
+                                     kEmitStoreBarrier));
+      // Create new context containing the receiver.
+      const intptr_t kNumContextVariables = 1;  // The receiver.
+      Value* allocated_context =
+          Bind(new AllocateContextInstr(node->token_pos(),
+                                        kNumContextVariables));
+      { LocalVariable* tmp_var = EnterTempLocalScope(allocated_context);
+        ValueGraphVisitor for_receiver(owner());
+        node->receiver()->Visit(&for_receiver);

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

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.

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

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

+        Append(for_receiver);
+        Value* receiver = for_receiver.value();
+        Value* tmp_val = Bind(new LoadLocalInstr(*tmp_var));
+        Do(new StoreVMFieldInstr(tmp_val,
+                                 Context::variable_offset(0),
+                                 receiver,
+                                 Type::ZoneHandle()));
+        AddInstruction(
+            new StoreContextInstr(Bind(ExitTempLocalScope(tmp_var))));

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

The StoreContext does not appear in the original code. I think you can just drop
the allocated context by using

Do(ExitTempLocalScope(context_var));

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

No idea why I have this StoreContextInstr here.

+      }
+      // Store new context.
+      tmp_val = Bind(new LoadLocalInstr(*tmp_var));
+      Do(new StoreInstanceFieldInstr(context_field,

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

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]

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!

+                                     tmp_val,
+                                     allocated_context,
+                                     kEmitStoreBarrier));
+      ReturnDefinition(ExitTempLocalScope(tmp_var));
+    }
   }
 }