[interpreter] Inline FastCloneShallowArrayStub into bytecode handler

[interpreter] Inline FastCloneShallowArrayStub into bytecode handler

The CreateArrayLiteral bytecode handler now directly inlines the FastCloneShallowArrayStub.

BUG=v8:4280
Committed: https://crrev.com/5deb0bc15788e2d553a720ce53b1ab3401823779
Cr-Commit-Position: refs/heads/master@{#39562}

[klaasb, 2016-09-15 14:12:57]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

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

Dry run: 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-15 14:22:20]

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

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

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[klaasb, 2016-09-15 14:28:56]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

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

Dry run: 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-15 14:55:07]

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

[commit-bot: I haz the power, 2016-09-15 14:55:08]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-09-15 15:10:43]

klaasb@google.com changed reviewers:
+ mstarzinger@chromium.org, rmcilroy@chromium.org

[klaasb, 2016-09-15 15:11:16]

Description was changed from

==========
[interpreter] Inline FastCloneShallowArrayStub

The CreateArrayLiteral bytecode handler now directly inlines the
FastCloneShallowArrayStub.

BUG=v8:4280
==========

to

==========
[interpreter] Inline FastCloneShallowArrayStub into bytecode handler

The CreateArrayLiteral bytecode handler now directly inlines the
FastCloneShallowArrayStub.

BUG=v8:4280
==========

[rmcilroy, 2016-09-16 08:52:27]

Nice, LGTM once comments are addressed.

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

https://codereview.chromium.org/2341743003/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:5708: AllocationSiteMode allocation_site_mode,
nit - allocation_site_mode at the end of parameters.

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-flags.h (right):

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-flags.h:17: class MustUseRuntimeBit : public
BitField8<bool, FlagsBits::kNext, 1> {};
nit - the other flags use a bit to specify using the fast-path. Could you do it
this way round for consistency.

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

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1884: __
BitFieldDecode<CreateArrayLiteralFlags::MustUseRuntimeBit>(
Last time I looked this emitted quite a few instructions for what could be just
an AND. Is this still the case or does it emit good code now?

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1894: __ Goto(&dispatch);
do the dispatch inline for both of these branches (so the fastpath doesn't need
to reload bytecode array from stack).

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1905: Node* constant_elements = __
LoadConstantPoolEntry(index);
Hmm, it's interesting that the runtime call needs the constant_elements while
the stub doesn't. Perhaps we should think of two bytecodes for these operations.
This is good for now, we can split them in a followup if necessary.

[klaasb, 2016-09-19 13:53:58]

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

https://codereview.chromium.org/2341743003/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:5708: AllocationSiteMode allocation_site_mode,
On 2016/09/16 08:52:26, rmcilroy wrote:
> nit - allocation_site_mode at the end of parameters.

Done.

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-flags.h (right):

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-flags.h:17: class MustUseRuntimeBit : public
BitField8<bool, FlagsBits::kNext, 1> {};
On 2016/09/16 08:52:26, rmcilroy wrote:
> nit - the other flags use a bit to specify using the fast-path. Could you do
it
> this way round for consistency.

Done.

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

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1884: __
BitFieldDecode<CreateArrayLiteralFlags::MustUseRuntimeBit>(
On 2016/09/16 08:52:26, rmcilroy wrote:
> Last time I looked this emitted quite a few instructions for what could be
just
> an AND. Is this still the case or does it emit good code now?

It still seems to emit a bunch of code (bytecode_flags in edi):
mov esi,edi
and esi,0x8
shr esi,3
jz ...
with just a Word32And:
test_w edi,0x8
jz ...

Done.

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1894: __ Goto(&dispatch);
On 2016/09/16 08:52:26, rmcilroy wrote:
> do the dispatch inline for both of these branches (so the fastpath doesn't
need
> to reload bytecode array from stack).

Done.

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1905: Node* constant_elements = __
LoadConstantPoolEntry(index);
On 2016/09/16 08:52:26, rmcilroy wrote:
> Hmm, it's interesting that the runtime call needs the constant_elements while
> the stub doesn't. Perhaps we should think of two bytecodes for these
operations.
> This is good for now, we can split them in a followup if necessary.

Acknowledged.

[Michael Starzinger, 2016-09-19 15:15:16]

mstarzinger@chromium.org changed reviewers:
+ epertoso@chromium.org

[Michael Starzinger, 2016-09-19 15:15:16]

LGTM modulo one comment, adding Enrico about this.

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

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1884: __
BitFieldDecode<CreateArrayLiteralFlags::MustUseRuntimeBit>(
On 2016/09/19 13:53:58, klaasb wrote:
> On 2016/09/16 08:52:26, rmcilroy wrote:
> > Last time I looked this emitted quite a few instructions for what could be
> just
> > an AND. Is this still the case or does it emit good code now?
> 
> It still seems to emit a bunch of code (bytecode_flags in edi):
> mov esi,edi
> and esi,0x8
> shr esi,3
> jz ...
> with just a Word32And:
> test_w edi,0x8
> jz ...
> 
> Done.

This looks dangerous to me. The version that correctly decoded the bit-field
guaranteed that the boolean bit to be either 0 or 1. I am not sure the version
that makes boolean bit be at position 3 is guaranteed to work on all backends.
I'll add Enrico to further comment (and confirm or deny) on this.

[epertoso, 2016-09-20 11:54:34]

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

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1884: __
BitFieldDecode<CreateArrayLiteralFlags::MustUseRuntimeBit>(
On 2016/09/19 15:15:16, Michael Starzinger wrote:
> On 2016/09/19 13:53:58, klaasb wrote:
> > On 2016/09/16 08:52:26, rmcilroy wrote:
> > > Last time I looked this emitted quite a few instructions for what could be
> > just
> > > an AND. Is this still the case or does it emit good code now?
> > 
> > It still seems to emit a bunch of code (bytecode_flags in edi):
> > mov esi,edi
> > and esi,0x8
> > shr esi,3
> > jz ...
> > with just a Word32And:
> > test_w edi,0x8
> > jz ...
> > 
> > Done.
> 
> This looks dangerous to me. The version that correctly decoded the bit-field
> guaranteed that the boolean bit to be either 0 or 1. I am not sure the version
> that makes boolean bit be at position 3 is guaranteed to work on all backends.
> I'll add Enrico to further comment (and confirm or deny) on this.

edi gets copied to esi to avoid modifying its value (it's used below).

The and followed by shr is a missing optimization from the ia32 instruction
selector (I'm sure the x64 IS is affected by this as well).

Using Word32And as a condition is safe, we treat is as a "compare the result
with zero", which should do the right thing for all of the instruction selectors
I have knowledge of (i.e. we emit ands for arm/arm64, use test on ia32/x64, and
compare the result with zero on mips).

[klaasb, 2016-09-20 13:42:43]

The CQ bit was checked by klaasb@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 13:42:53]

Dry run: 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-20 14:32:52]

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

[commit-bot: I haz the power, 2016-09-20 14:32:53]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-09-20 18:01:28]

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

https://codereview.chromium.org/2341743003/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1884: __
BitFieldDecode<CreateArrayLiteralFlags::MustUseRuntimeBit>(
On 2016/09/20 11:54:33, epertoso wrote:
> On 2016/09/19 15:15:16, Michael Starzinger wrote:
> > On 2016/09/19 13:53:58, klaasb wrote:
> > > On 2016/09/16 08:52:26, rmcilroy wrote:
> > > > Last time I looked this emitted quite a few instructions for what could
be
> > > just
> > > > an AND. Is this still the case or does it emit good code now?
> > > 
> > > It still seems to emit a bunch of code (bytecode_flags in edi):
> > > mov esi,edi
> > > and esi,0x8
> > > shr esi,3
> > > jz ...
> > > with just a Word32And:
> > > test_w edi,0x8
> > > jz ...
> > > 
> > > Done.
> > 
> > This looks dangerous to me. The version that correctly decoded the bit-field
> > guaranteed that the boolean bit to be either 0 or 1. I am not sure the
version
> > that makes boolean bit be at position 3 is guaranteed to work on all
backends.
> > I'll add Enrico to further comment (and confirm or deny) on this.
> 
> edi gets copied to esi to avoid modifying its value (it's used below).
> 
> The and followed by shr is a missing optimization from the ia32 instruction
> selector (I'm sure the x64 IS is affected by this as well).
> 
> Using Word32And as a condition is safe, we treat is as a "compare the result
> with zero", which should do the right thing for all of the instruction
selectors
> I have knowledge of (i.e. we emit ands for arm/arm64, use test on ia32/x64,
and
> compare the result with zero on mips).

Thanks for the explanation Enrico!

[klaasb, 2016-09-20 18:01:36]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-09-20 18:01:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2341743003/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-20 18:01:48]

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-20 18:04:26]

Description was changed from

==========
[interpreter] Inline FastCloneShallowArrayStub into bytecode handler

The CreateArrayLiteral bytecode handler now directly inlines the
FastCloneShallowArrayStub.

BUG=v8:4280
==========

to

==========
[interpreter] Inline FastCloneShallowArrayStub into bytecode handler

The CreateArrayLiteral bytecode handler now directly inlines the
FastCloneShallowArrayStub.

BUG=v8:4280
==========

[commit-bot: I haz the power, 2016-09-20 18:04:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-20 18:05:01]

Description was changed from

==========
[interpreter] Inline FastCloneShallowArrayStub into bytecode handler

The CreateArrayLiteral bytecode handler now directly inlines the
FastCloneShallowArrayStub.

BUG=v8:4280
==========

to

==========
[interpreter] Inline FastCloneShallowArrayStub into bytecode handler

The CreateArrayLiteral bytecode handler now directly inlines the
FastCloneShallowArrayStub.

BUG=v8:4280

Committed: https://crrev.com/5deb0bc15788e2d553a720ce53b1ab3401823779
Cr-Commit-Position: refs/heads/master@{#39562}
==========

[commit-bot: I haz the power, 2016-09-20 18:05:02]

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