[stubs] Port ToName stub to TurboFan.

[stubs] Port ToName stub to TurboFan.

Migrate the platform ToName stub to TurboFan.

BUG=v8:5049
Committed: https://crrev.com/f439fcab698a5b4f3c914d85137a0a5f9fa2326b
Cr-Commit-Position: refs/heads/master@{#39315}

[alegil02, 2016-09-01 14:33:41]

Description was changed from

==========
[stubs] Port ToName stub to TurboFan.

Migrate the platform ToName stub to TurboFan.

BUG=v8:5049
==========

to

==========
[stubs] Port ToName stub to TurboFan.

Migrate the platform ToName stub to TurboFan.

BUG=v8:5049
==========

[alegil02, 2016-09-01 14:33:41]

alexander.gilday2@arm.com changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2016-09-02 04:36:26]

While you are at it, can you turn this code stub into a turbofan builtin please?
There's no reason to have it as a stub.

https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc#newcode4692
src/code-stubs.cc:4692: var_result.Bind(assembler->CallStub(callable, context,
value));
You can tail call to the stub here.

https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc#newcode4715
src/code-stubs.cc:4715: var_result.Bind(assembler->CallRuntime(Runtime::kToName,
context, value));
Likewise you can tail call to the runtime here.

[alegil02, 2016-09-02 13:42:05]

On 2016/09/02 04:36:26, Benedikt Meurer wrote:
> While you are at it, can you turn this code stub into a turbofan builtin
please?
> There's no reason to have it as a stub.
> 
> https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc
> File src/code-stubs.cc (right):
> 
>
https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc#newcode4692
> src/code-stubs.cc:4692: var_result.Bind(assembler->CallStub(callable, context,
> value));
> You can tail call to the stub here.
> 
>
https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc#newcode4715
> src/code-stubs.cc:4715:
var_result.Bind(assembler->CallRuntime(Runtime::kToName,
> context, value));
> Likewise you can tail call to the runtime here.

I can turn it into a builtin, however if I take the same route as StringToNumber
by creating a helper for inlining then I believe it wouldn't be possible to tail
call to the stub or runtime in these cases?

[Benedikt Meurer, 2016-09-03 17:58:05]

On 2016/09/02 13:42:05, alegil02 wrote:
> On 2016/09/02 04:36:26, Benedikt Meurer wrote:
> > While you are at it, can you turn this code stub into a turbofan builtin
> please?
> > There's no reason to have it as a stub.
> > 
> > https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc
> > File src/code-stubs.cc (right):
> > 
> >
>
https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc#newcode4692
> > src/code-stubs.cc:4692: var_result.Bind(assembler->CallStub(callable,
context,
> > value));
> > You can tail call to the stub here.
> > 
> >
>
https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc#newcode4715
> > src/code-stubs.cc:4715:
> var_result.Bind(assembler->CallRuntime(Runtime::kToName,
> > context, value));
> > Likewise you can tail call to the runtime here.
> 
> I can turn it into a builtin, however if I take the same route as
StringToNumber
> by creating a helper for inlining then I believe it wouldn't be possible to
tail
> call to the stub or runtime in these cases?

You don't need the helper. Just turn the code stub into a TFS builtin.

[alegil02, 2016-09-05 08:20:17]

On 2016/09/03 17:58:05, Benedikt Meurer wrote:
> On 2016/09/02 13:42:05, alegil02 wrote:
> > On 2016/09/02 04:36:26, Benedikt Meurer wrote:
> > > While you are at it, can you turn this code stub into a turbofan builtin
> > please?
> > > There's no reason to have it as a stub.
> > > 
> > > https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc
> > > File src/code-stubs.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc#newcode4692
> > > src/code-stubs.cc:4692: var_result.Bind(assembler->CallStub(callable,
> context,
> > > value));
> > > You can tail call to the stub here.
> > > 
> > >
> >
>
https://codereview.chromium.org/2302923002/diff/1/src/code-stubs.cc#newcode4715
> > > src/code-stubs.cc:4715:
> > var_result.Bind(assembler->CallRuntime(Runtime::kToName,
> > > context, value));
> > > Likewise you can tail call to the runtime here.
> > 
> > I can turn it into a builtin, however if I take the same route as
> StringToNumber
> > by creating a helper for inlining then I believe it wouldn't be possible to
> tail
> > call to the stub or runtime in these cases?
> 
> You don't need the helper. Just turn the code stub into a TFS builtin.

Ah I was creating a helper so it could be inlined in the other stubs, for
example there was a //TODO in the ForInFilter stub to inline it.

[Benedikt Meurer, 2016-09-05 08:22:02]

bmeurer@chromium.org changed reviewers:
+ ishell@chromium.org

[Benedikt Meurer, 2016-09-05 08:22:03]

Adding ishell@ for review.

I wasn't aware of the inlining idea (to be honest this is not my area of
expertise currently), so I'll leave it to Igor to comment on that.

[rmcilroy, 2016-09-05 09:22:18]

On 2016/09/05 08:22:03, Benedikt Meurer wrote:
> Adding ishell@ for review.
> 
> I wasn't aware of the inlining idea (to be honest this is not my area of
> expertise currently), so I'll leave it to Igor to comment on that.

This could also be inlined in Interpreter::DoToName() possibly (not sure how it
would effect performance though).

[alegil02, 2016-09-05 16:23:19]

On 2016/09/05 09:22:18, rmcilroy wrote:
> On 2016/09/05 08:22:03, Benedikt Meurer wrote:
> > Adding ishell@ for review.
> > 
> > I wasn't aware of the inlining idea (to be honest this is not my area of
> > expertise currently), so I'll leave it to Igor to comment on that.
> 
> This could also be inlined in Interpreter::DoToName() possibly (not sure how
it
> would effect performance though).

I have inlined the function into Interpreter::DoToName too, though I haven't
been able to address the tail call issue yet. One possibility is to add a flag
as a parameter in the helper which enables the use of tail call for the builtin
but defaults to normal calls for generating the inline versions. Thoughts?

[Benedikt Meurer, 2016-09-06 03:22:05]

On 2016/09/05 16:23:19, alegil02 wrote:
> On 2016/09/05 09:22:18, rmcilroy wrote:
> > On 2016/09/05 08:22:03, Benedikt Meurer wrote:
> > > Adding ishell@ for review.
> > > 
> > > I wasn't aware of the inlining idea (to be honest this is not my area of
> > > expertise currently), so I'll leave it to Igor to comment on that.
> > 
> > This could also be inlined in Interpreter::DoToName() possibly (not sure how
> it
> > would effect performance though).
> 
> I have inlined the function into Interpreter::DoToName too, though I haven't
> been able to address the tail call issue yet. One possibility is to add a flag
> as a parameter in the helper which enables the use of tail call for the
builtin
> but defaults to normal calls for generating the inline versions. Thoughts?

You don't need to force this to a tail call. If it'd be just the stub/builtin
itself, then a tail call would be nice, but now with the benefit of inlining,
the tail call is a lot less relevant (if at all).

[Igor Sheludko, 2016-09-06 08:23:04]

lgtm with nits:

https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:2226: Label is_number(this, Label::kDeferred);
I would suggest not to use deferred labels when rewriting hand written assembly
to TurboFan since the deferred blocks are in most cases penalized by gap moves.
Please check what kind of assembly does TF generate.

https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:2229: Label not_name(this, Label::kDeferred);
Same here.

https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:2230: Node* value_instance_type =
LoadInstanceType(value);
Please add
  STATIC_ASSERT(FIRST_NAME_TYPE == FIRST_TYPE);

https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:2249: Label not_oddball(this, Label::kDeferred);
Same here.

[alegil02, 2016-09-06 15:48:48]

On 2016/09/06 08:23:04, Igor Sheludko wrote:
> lgtm with nits:
> 
>
https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler.cc
> File src/code-stub-assembler.cc (right):
> 
>
https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler...
> src/code-stub-assembler.cc:2226: Label is_number(this, Label::kDeferred);
> I would suggest not to use deferred labels when rewriting hand written
assembly
> to TurboFan since the deferred blocks are in most cases penalized by gap
moves.
> Please check what kind of assembly does TF generate.
> 
>
https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler...
> src/code-stub-assembler.cc:2229: Label not_name(this, Label::kDeferred);
> Same here.
> 
>
https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler...
> src/code-stub-assembler.cc:2230: Node* value_instance_type =
> LoadInstanceType(value);
> Please add
>   STATIC_ASSERT(FIRST_NAME_TYPE == FIRST_TYPE);
> 
>
https://codereview.chromium.org/2302923002/diff/20001/src/code-stub-assembler...
> src/code-stub-assembler.cc:2249: Label not_oddball(this, Label::kDeferred);
> Same here.

Thanks, Igor. Fixed nits, PTAL.

[Igor Sheludko, 2016-09-06 15:51:44]

lgtm

[alegil02, 2016-09-06 15:53:05]

The CQ bit was checked by alexander.gilday2@arm.com

[commit-bot: I haz the power, 2016-09-06 15:53:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 16:00:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 16:00:25]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23427)

[alegil02, 2016-09-06 16:10:11]

On 2016/09/06 16:00:25, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_presubmit on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23427)

It seems I need a reviewer that owns interpreter.cc to lgtm this patch. Which
reviewer would be most applicable?

[Igor Sheludko, 2016-09-06 16:10:56]

ishell@chromium.org changed reviewers:
+ alexander.gilday2@arm.com, rmcilroy@chromium.org
- Alexander.Gilday2@arm.com

[Igor Sheludko, 2016-09-06 16:10:57]

Ross, PTAL interpreter part.

[rmcilroy, 2016-09-07 13:57:35]

Interpreter LGTM with two nits. Thanks.

https://codereview.chromium.org/2302923002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2302923002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1306: Node* accumulator = __ GetAccumulator();
nit - rename "accumulator" to "object"

https://codereview.chromium.org/2302923002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1308: Node* result = assembler->ToName(context,
accumulator);
nit - use "__ ToName()" instead of "assembler->ToName"

[alegil02, 2016-09-09 15:56:06]

The CQ bit was checked by alexander.gilday2@arm.com

[alegil02, 2016-09-09 15:56:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2302923002/#ps60001
(title: "Addressed nits.")

[commit-bot: I haz the power, 2016-09-09 15:56:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 16:23:29]

Description was changed from

==========
[stubs] Port ToName stub to TurboFan.

Migrate the platform ToName stub to TurboFan.

BUG=v8:5049
==========

to

==========
[stubs] Port ToName stub to TurboFan.

Migrate the platform ToName stub to TurboFan.

BUG=v8:5049
==========

[commit-bot: I haz the power, 2016-09-09 16:23:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-09 16:23:52]

Description was changed from

==========
[stubs] Port ToName stub to TurboFan.

Migrate the platform ToName stub to TurboFan.

BUG=v8:5049
==========

to

==========
[stubs] Port ToName stub to TurboFan.

Migrate the platform ToName stub to TurboFan.

BUG=v8:5049

Committed: https://crrev.com/f439fcab698a5b4f3c914d85137a0a5f9fa2326b
Cr-Commit-Position: refs/heads/master@{#39315}
==========

[commit-bot: I haz the power, 2016-09-09 16:23:53]

Patchset 4 (id:??) landed as
https://crrev.com/f439fcab698a5b4f3c914d85137a0a5f9fa2326b
Cr-Commit-Position: refs/heads/master@{#39315}