VM: Implement closure calls as instance calls.

VM: Implement closure calls as instance calls.

Conceptually f() becomes f.call() where the .call method performs the actual
closure call. The closure call itself is implemented without a stub.

The check if the object called is a closure becomes a class-id check
which can be hoisted out of loops.

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

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

[srdjan, 2014-04-09 18:28:42]

lgtm

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:476: InlineInstanceCalls();
Any reason for changing the order?

[srdjan, 2014-04-09 18:29:51]

On 2014/04/09 18:28:42, srdjan wrote:
> lgtm
> 
>
https://codereview.chromium.org/227723002/diff/60001/runtime/vm/flow_graph_in...
> File runtime/vm/flow_graph_inliner.cc (right):
> 
>
https://codereview.chromium.org/227723002/diff/60001/runtime/vm/flow_graph_in...
> runtime/vm/flow_graph_inliner.cc:476: InlineInstanceCalls();
> Any reason for changing the order?

LGTM after making sure that there a no performance regressions on closure based
benchmarks.

[zra, 2014-04-09 19:30:01]

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/stub_code_arm.cc
File runtime/vm/stub_code_arm.cc (left):

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:739: void
StubCode::GenerateCallClosureFunctionStub(Assembler* assembler) {
Please also remove from arm64.

[Ivan Posva, 2014-04-10 04:38:55]

I would like to get an answer for the decision to unconditionally dispatching in
case of a signature function before this is ready for LGTM.

Thanks,
-Ivan

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

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18044: } else if (function.is_visible() ||
FLAG_verbose_stacktrace) {
What if the function is not visible, but it has inlined a couple of frames that
are visible? Wouldn't we start dropping visible functions below?

Please fix in a separate CL.

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:1430: result = new ClosureCallNode(token_pos, getter_call,
args);
You might want to add a comment that this is where the call() method is being
synthesized. Maybe even add that the function name is "call", no?

Actually, what if you end up dispatching on a different field from the signature
class?

[Florian Schneider, 2014-04-10 08:43:56]

On 2014/04/10 04:38:55, Ivan Posva wrote:
> I would like to get an answer for the decision to unconditionally dispatching
in
> case of a signature function before this is ready for LGTM.
> 

Where would I dispatch on a signature function?

> Thanks,
> -Ivan
> 
> https://codereview.chromium.org/227723002/diff/60001/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/227723002/diff/60001/runtime/vm/object.cc#new...
> runtime/vm/object.cc:18044: } else if (function.is_visible() ||
> FLAG_verbose_stacktrace) {
> What if the function is not visible, but it has inlined a couple of frames
that
> are visible? Wouldn't we start dropping visible functions below?
> 
> Please fix in a separate CL.
> 
> https://codereview.chromium.org/227723002/diff/60001/runtime/vm/parser.cc
> File runtime/vm/parser.cc (right):
> 
>
https://codereview.chromium.org/227723002/diff/60001/runtime/vm/parser.cc#new...
> runtime/vm/parser.cc:1430: result = new ClosureCallNode(token_pos,
getter_call,
> args);
> You might want to add a comment that this is where the call() method is being
> synthesized. Maybe even add that the function name is "call", no?
> 
> Actually, what if you end up dispatching on a different field from the
signature
> class?

[Florian Schneider, 2014-04-10 08:49:25]

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:476: InlineInstanceCalls();
On 2014/04/09 18:28:43, srdjan wrote:
> Any reason for changing the order?

The reason is that dispatchers are instance calls, so if we want to inline other
calls inside dispatchers at the current depth, we need to inline those first.

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

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/object.cc#new...
runtime/vm/object.cc:18044: } else if (function.is_visible() ||
FLAG_verbose_stacktrace) {
On 2014/04/10 04:38:55, Ivan Posva wrote:
> What if the function is not visible, but it has inlined a couple of frames
that
> are visible? Wouldn't we start dropping visible functions below?
> 
> Please fix in a separate CL.

Yes, that should be fixed too.

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:1430: result = new ClosureCallNode(token_pos, getter_call,
args);
On 2014/04/10 04:38:55, Ivan Posva wrote:
> You might want to add a comment that this is where the call() method is being
> synthesized. Maybe even add that the function name is "call", no?
> 
> Actually, what if you end up dispatching on a different field from the
signature
> class?

Done.

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/stub_code_arm.cc
File runtime/vm/stub_code_arm.cc (left):

https://codereview.chromium.org/227723002/diff/60001/runtime/vm/stub_code_arm...
runtime/vm/stub_code_arm.cc:739: void
StubCode::GenerateCallClosureFunctionStub(Assembler* assembler) {
On 2014/04/09 19:30:01, zra wrote:
> Please also remove from arm64.

Done.

[Florian Schneider, 2014-04-10 08:54:38]

On 2014/04/09 18:29:51, srdjan wrote:
> On 2014/04/09 18:28:42, srdjan wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/227723002/diff/60001/runtime/vm/flow_graph_in...
> > File runtime/vm/flow_graph_inliner.cc (right):
> > 
> >
>
https://codereview.chromium.org/227723002/diff/60001/runtime/vm/flow_graph_in...
> > runtime/vm/flow_graph_inliner.cc:476: InlineInstanceCalls();
> > Any reason for changing the order?
> 
> LGTM after making sure that there a no performance regressions on closure
based
> benchmarks.

Remember that this CL is not the final chapter on this topic yet, and currently
there is still unnecessary work done at every call:

I plan to remove the implicit setting of the context and loading the shared
function object and make it explicit in the IL. This enables load elimination
for them.

The closure object does not need to be pushed as argument - instead
ClosureCallInstr just takes the shared function as input operand.

[Florian Schneider, 2014-04-10 10:56:36]

Committed patchset #6 manually as r34917 (presubmit successful).

[Ivan Posva, 2014-04-10 21:01:02]

On 2014/04/10 08:43:56, Florian Schneider wrote:
> On 2014/04/10 04:38:55, Ivan Posva wrote:
> > I would like to get an answer for the decision to unconditionally
dispatching
> in
> > case of a signature function before this is ready for LGTM.
> > 
> 
> Where would I dispatch on a signature function?

Bad terminology on my part. I meant to say an invoke field dispatcher in
signature classes. It looks to me as if your commit now does check for the
dispatcher being named call before assuming closure calls.

-Ivan

> 
> > Thanks,
> > -Ivan
> > 
> > https://codereview.chromium.org/227723002/diff/60001/runtime/vm/object.cc
> > File runtime/vm/object.cc (right):
> > 
> >
>
https://codereview.chromium.org/227723002/diff/60001/runtime/vm/object.cc#new...
> > runtime/vm/object.cc:18044: } else if (function.is_visible() ||
> > FLAG_verbose_stacktrace) {
> > What if the function is not visible, but it has inlined a couple of frames
> that
> > are visible? Wouldn't we start dropping visible functions below?
> > 
> > Please fix in a separate CL.
> > 
> > https://codereview.chromium.org/227723002/diff/60001/runtime/vm/parser.cc
> > File runtime/vm/parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/227723002/diff/60001/runtime/vm/parser.cc#new...
> > runtime/vm/parser.cc:1430: result = new ClosureCallNode(token_pos,
> getter_call,
> > args);
> > You might want to add a comment that this is where the call() method is
being
> > synthesized. Maybe even add that the function name is "call", no?
> > 
> > Actually, what if you end up dispatching on a different field from the
> signature
> > class?