VM: Explicitly load function and context before calling a closure.

VM: Explicitly load function and context before calling a closure.

Before these were implicitly loaded as part of the closure calling code sequence.

This CL makes those loads canditates for load elimination. The context is also
explictly stored before the call.

R=regis@google.com, srdjan@google.com

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

[srdjan, 2014-04-30 15:41:58]

LGTM after OK from Regis on object.cc change

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

https://codereview.chromium.org/265443002/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2417: 
Why the empty line?

https://codereview.chromium.org/265443002/diff/1/runtime/vm/flow_graph_compil...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/265443002/diff/1/runtime/vm/flow_graph_compil...
runtime/vm/flow_graph_compiler.h:421: intptr_t stack_adjustment = 0);
Since this is public, please explain briefly what stack_adjustment is used for..

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:13851: if (cls.raw() == Object::function_class() &&
cls.IsFunctionClass()

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:13853: return Object::dynamic_type();
I would like to have Regis look into this change.

[Florian Schneider, 2014-04-30 16:02:18]

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

https://codereview.chromium.org/265443002/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:2417: 
On 2014/04/30 15:41:58, srdjan wrote:
> Why the empty line?

Done.

https://codereview.chromium.org/265443002/diff/1/runtime/vm/flow_graph_compil...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/265443002/diff/1/runtime/vm/flow_graph_compil...
runtime/vm/flow_graph_compiler.h:421: intptr_t stack_adjustment = 0);
On 2014/04/30 15:41:58, srdjan wrote:
> Since this is public, please explain briefly what stack_adjustment is used
for..

Done. Added comment to the implementation and renamed to input_count: It's the
number of input operands (as opposed to pushed arguments) of the call.
ClosureCallInstr has now 1 input operand (the Function object).

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:13851: if (cls.raw() == Object::function_class() &&
On 2014/04/30 15:41:58, srdjan wrote:
> cls.IsFunctionClass()

This seems equivalent and consistent with the code above, so I'd rather leave it
as is.

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:13853: return Object::dynamic_type();
On 2014/04/30 15:41:58, srdjan wrote:
> I would like to have Regis look into this change.

Now that I look at it again, actually, this should return 
Isolate::Current()->object_store()->function_type();

The problem here is that the flow-graph propagation want to canonicalize a type
that's stored in the VM isolate heap (read-only).

This occurs because we pass a Function value around in the IL (as input operand
to closure calls).

[Florian Schneider, 2014-04-30 16:05:23]

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:13853: return Object::dynamic_type();
On 2014/04/30 16:02:19, Florian Schneider wrote:
> On 2014/04/30 15:41:58, srdjan wrote:
> > I would like to have Regis look into this change.
> 
> Now that I look at it again, actually, this should return 
> Isolate::Current()->object_store()->function_type();
> 

No, not function_type, but the type corresponding to the RawFunction VM class,
not the Dart class.

[regis, 2014-04-30 16:46:38]

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:13853: return Object::dynamic_type();
On 2014/04/30 16:05:24, Florian Schneider wrote:
> On 2014/04/30 16:02:19, Florian Schneider wrote:
> > On 2014/04/30 15:41:58, srdjan wrote:
> > > I would like to have Regis look into this change.
> > 
> > Now that I look at it again, actually, this should return 
> > Isolate::Current()->object_store()->function_type();
> > 
> 
> No, not function_type, but the type corresponding to the RawFunction VM class,
> not the Dart class.

I fail to see what problem you are trying to solve here.
Why do you need a canonical Function type across all isolates?

[Florian Schneider, 2014-04-30 17:42:22]

On 2014/04/30 16:46:38, regis wrote:
> https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/265443002/diff/1/runtime/vm/object.cc#newcode...
> runtime/vm/object.cc:13853: return Object::dynamic_type();
> On 2014/04/30 16:05:24, Florian Schneider wrote:
> > On 2014/04/30 16:02:19, Florian Schneider wrote:
> > > On 2014/04/30 15:41:58, srdjan wrote:
> > > > I would like to have Regis look into this change.
> > > 
> > > Now that I look at it again, actually, this should return 
> > > Isolate::Current()->object_store()->function_type();
> > > 
> > 
> > No, not function_type, but the type corresponding to the RawFunction VM
class,
> > not the Dart class.
> 
> I fail to see what problem you are trying to solve here.
> Why do you need a canonical Function type across all isolates?

The type propagation in the compiler calls NewNonParameterizedType with the
VM-internal Function class which fails for Function since this class is stored
in the VM isolate.

#4  0x083a4baf in dart::Type::Canonicalize (this=0xedf11ff8, trail=0x0) at
runtime/vm/object.cc:13855
#5  0x08399f2f in dart::Type::NewNonParameterizedType (type_class=...) at
runtime/vm/object.cc:13553
#6  0x082add80 in dart::CompileType::ToAbstractType (this=0xee0d9c6c) at
runtime/vm/flow_graph_type_propagator.cc:556

[regis, 2014-04-30 18:23:15]

https://codereview.chromium.org/265443002/diff/10001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/265443002/diff/10001/runtime/vm/object.h#newc...
runtime/vm/object.h:438: static RawType* function_type() { return
function_type_; }
You should not introduce this type. This is not a Dart type.
You will never do any type check on it. The type of the function field in a
closure should either be null or dynamic.
This is similar to the parent field in a context. It has no Dart type. I think
it is null in that case.

https://codereview.chromium.org/265443002/diff/10001/runtime/vm/object.h#newc...
runtime/vm/object.h:595: static RawType* function_type_;  // Type of the
'Function' vm class.
ditto

[Florian Schneider, 2014-04-30 18:57:33]

https://codereview.chromium.org/265443002/diff/10001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/265443002/diff/10001/runtime/vm/object.h#newc...
runtime/vm/object.h:438: static RawType* function_type() { return
function_type_; }
On 2014/04/30 18:23:15, regis wrote:
> You should not introduce this type. This is not a Dart type.
> You will never do any type check on it. The type of the function field in a
> closure should either be null or dynamic.
> This is similar to the parent field in a context. It has no Dart type. I think
> it is null in that case.

Done. I changed flow_graph_type_propagator instead to never create a
AbstractType from a value with kFunctionCid.

[regis, 2014-04-30 19:33:04]

LGTM

[Florian Schneider, 2014-04-30 20:14:16]

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

[zra, 2014-04-30 23:03:10]

On 2014/04/30 20:14:16, Florian Schneider wrote:
> Committed patchset #4 manually as r35595 (presubmit successful).

DBC.

Please keep an eye out for things that have already been implemented on arm64,
e.g. the ClosureCallInstr. I'll fix it this time.