Port FastCloneShallowArrayStub to Turbofan

Port FastCloneShallowArrayStub to Turbofan

Refactors CodeStubAssembler::AllocateJSArray to share code.

BUG=chromium:608675
Committed: https://crrev.com/e2455873e6146d57b453ce15eab7cd957bbca08b
Cr-Commit-Position: refs/heads/master@{#39550}

[klaasb, 2016-09-06 09:57:28]

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

[commit-bot: I haz the power, 2016-09-06 09:57:36]

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

[klaasb, 2016-09-06 10:00:40]

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

[commit-bot: I haz the power, 2016-09-06 10:00:46]

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

[klaasb, 2016-09-06 10:05:06]

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

[commit-bot: I haz the power, 2016-09-06 10:05:14]

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-06 10:23:44]

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

[commit-bot: I haz the power, 2016-09-06 10:23:45]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8274)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[klaasb, 2016-09-06 15:01:13]

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

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

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-06 15:23:09]

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

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

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

[klaasb, 2016-09-07 17:54:17]

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

[commit-bot: I haz the power, 2016-09-07 17:54:22]

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-07 18:21:37]

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

[commit-bot: I haz the power, 2016-09-07 18:21:38]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-09-12 11:03:28]

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

[commit-bot: I haz the power, 2016-09-12 11:03:32]

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-12 11:07:42]

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

[commit-bot: I haz the power, 2016-09-12 11:07:43]

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-12 11:12:14]

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

[commit-bot: I haz the power, 2016-09-12 11:12:22]

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-12 11:45:11]

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

[commit-bot: I haz the power, 2016-09-12 11:45:12]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-09-12 15:01:12]

Description was changed from

==========
WIP port fastcloneshallowarraystub to TF

not finished, lots of refactoring to do, moving stuff to
CodeStubAssembler and a few bugs

BUG=chromium:608675
==========

to

==========
Port FastCloneShallowArrayStub to Turbofan

Refactors CodeStubAssembler::AllocateJSArray to share code.

BUG=chromium:608675
==========

[klaasb, 2016-09-12 15:21:10]

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

[klaasb, 2016-09-12 16:41:56]

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

[commit-bot: I haz the power, 2016-09-12 16:42:04]

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-12 16:43:36]

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

[commit-bot: I haz the power, 2016-09-12 16:43:38]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12561)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12556)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12493)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23835)

[klaasb, 2016-09-13 10:12:24]

klaasb@google.com changed reviewers:
+ danno@chromium.org

[klaasb, 2016-09-13 10:13:14]

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

[commit-bot: I haz the power, 2016-09-13 10:13:24]

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-13 10:14:54]

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

[commit-bot: I haz the power, 2016-09-13 10:14:55]

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...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/23900)

[klaasb, 2016-09-13 10:49:47]

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

[commit-bot: I haz the power, 2016-09-13 10:49:50]

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-13 11:04:53]

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

[commit-bot: I haz the power, 2016-09-13 11:04:54]

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...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[klaasb, 2016-09-13 12:28:43]

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

[commit-bot: I haz the power, 2016-09-13 12:28:46]

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-13 12:59:48]

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

[commit-bot: I haz the power, 2016-09-13 12:59:50]

Dry run: This issue passed the CQ dry run.

[klaasb, 2016-09-13 13:41:04]

Please take a look.

I tried reducing register pressure for the stub, but even reloading everything
from allocation_site on use and reducing life times, doesn't reduce pressure
enough to remove the frame currently needed.

Turbofan doesn't seem to use the same addressing modes for CMP here as
Crankshaft does, e.g. where Crankshaft uses:
cmp [eax-0x1],0x580041dd

TF needs three instructions and two registers: 
mov eax,[eax-0x1]
mov ecx,0x580041dd
cmp ecx,eax

TF does use that (e.g. cmp [ebx+ecx*1+0x4],0xfff7ffff) for
CopyFixedArrayElements, so maybe I'm just generating the code in an
unoptimizable way?

[rmcilroy, 2016-09-13 20:29:59]

Overall looks good. A couple of comments.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2304573004/diff/280001/src/code-stub-assemble...
src/code-stub-assembler.cc:1381: Node*
CodeStubAssembler::AllocateEmptyJSArray(ElementsKind kind,
Maybe we should call this something like AllocateUninitializedJSArray to avoid
confusion with a valid empty array (since this won't be valid until elements is
set AFAIU). If so, I would just keep AllocateFilledJSArray as AllocateJSArray to
make it obvious this is the one everyone should use unless they know better.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stub-assemble...
src/code-stub-assembler.cc:1441: Node* elements = InnerAllocate(array,
elements_offset);
Do you do this twice? There is an InnerAllocate for AllocateEmptyJSArray if
capacity is nullptr. Maybe just avoid the capacity parameter on
AllocateEmptyJSArray and always do it here?

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

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5670: if (assembler->Is64()) {
Any idea why this is different on 64bit?

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5732: assembler->Comment("load capacity");
not sure if these "load.." comments add much. I'm easy either way though.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5749: assembler->Comment("non fixed path");
I don't think this is non-fixed, it's a fixed double arrays isn't it? Should it
instead be "fast double elements path"? Is there anything we can "dcheck" (in
FLAG_debug_code) that the elements_map equals here?

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5757: assembler->Bind(&fixed);
ditto - should this be fast_elements (and similar comment on Comment(...)

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5767: assembler->Bind(&fixed_cow);
fixed_cow_elements

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5789: Heap::kEmptyFixedArrayRootIndex);
Would it be worth merging the allocations for fixed cow and zero capacity (i.e.,
having length and the elements object be Variables set in the branches, then
merging to a common allocation branch which does the allocation?

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.h#newco...
src/code-stubs.h:1278: AllocationSiteMode allocation_site_mode =
DONT_TRACK_ALLOCATION_SITE);
no need for the default parameter here

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.h#newco...
src/code-stubs.h:1287: static compiler::Node* NonEmptyShallowClone(
This should probably just be an anonymous namespace function in the cc file
rather than a private static function in the class (seems to be the pattern for
other code stubs).

[klaasb, 2016-09-14 18:24:52]

PTAL

https://codereview.chromium.org/2304573004/diff/280001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2304573004/diff/280001/src/code-stub-assemble...
src/code-stub-assembler.cc:1381: Node*
CodeStubAssembler::AllocateEmptyJSArray(ElementsKind kind,
On 2016/09/13 20:29:59, rmcilroy wrote:
> Maybe we should call this something like AllocateUninitializedJSArray to avoid
> confusion with a valid empty array (since this won't be valid until elements
is
> set AFAIU). If so, I would just keep AllocateFilledJSArray as AllocateJSArray
to
> make it obvious this is the one everyone should use unless they know better.

Done.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stub-assemble...
src/code-stub-assembler.cc:1441: Node* elements = InnerAllocate(array,
elements_offset);
On 2016/09/13 20:29:58, rmcilroy wrote:
> Do you do this twice? There is an InnerAllocate for AllocateEmptyJSArray if
> capacity is nullptr. Maybe just avoid the capacity parameter on
> AllocateEmptyJSArray and always do it here?

We still want to allocate space for the array and the elements at the same time,
so we'd still need the parameter for that.
I changed AllocateUninitializedJSArray to return both which also gets rid of the
elements_offset calculation here and in the stub.

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

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5670: if (assembler->Is64()) {
On 2016/09/13 20:29:59, rmcilroy wrote:
> Any idea why this is different on 64bit?

This relates to the comment (now) at
https://cs.chromium.org/chromium/src/v8/src/code-stub-assembler.h?l=53
It's just passed on to ElementOffsetFromIndex.

We don't really need to untag the length, as we need it tagged to store it
anyway and we don't do any arithmetic with it anyway. The current state was the
last attempt to make it a bit more consistent, but I've changed it to have the
ParameterMode argument on AllocateUninitialized(/Empty)JSArray refer only to
capacity.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5732: assembler->Comment("load capacity");
On 2016/09/13 20:29:59, rmcilroy wrote:
> not sure if these "load.." comments add much. I'm easy either way though.

Yes, they were mostly for my benefit when looking at the disassembly. Removed.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5749: assembler->Comment("non fixed path");
On 2016/09/13 20:29:59, rmcilroy wrote:
> I don't think this is non-fixed, it's a fixed double arrays isn't it? Should
it
> instead be "fast double elements path"? Is there anything we can "dcheck" (in
> FLAG_debug_code) that the elements_map equals here?

Done.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5757: assembler->Bind(&fixed);
On 2016/09/13 20:29:59, rmcilroy wrote:
> ditto - should this be fast_elements (and similar comment on Comment(...)

Done.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5767: assembler->Bind(&fixed_cow);
On 2016/09/13 20:29:59, rmcilroy wrote:
> fixed_cow_elements

Renamed to cow_elements, to mirror fast_elements.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5789: Heap::kEmptyFixedArrayRootIndex);
On 2016/09/13 20:29:59, rmcilroy wrote:
> Would it be worth merging the allocations for fixed cow and zero capacity
(i.e.,
> having length and the elements object be Variables set in the branches, then
> merging to a common allocation branch which does the allocation?

I thought about that before, but seem to have forgotten to try it. It certainly
reduces code size and removes a load of a spilled value. Done.

Sharing more code at runtime for the other two branches is not easily done,
however.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.h#newco...
src/code-stubs.h:1278: AllocationSiteMode allocation_site_mode =
DONT_TRACK_ALLOCATION_SITE);
On 2016/09/13 20:29:59, rmcilroy wrote:
> no need for the default parameter here

Done.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.h#newco...
src/code-stubs.h:1287: static compiler::Node* NonEmptyShallowClone(
On 2016/09/13 20:29:59, rmcilroy wrote:
> This should probably just be an anonymous namespace function in the cc file
> rather than a private static function in the class (seems to be the pattern
for
> other code stubs).

Done.

[klaasb, 2016-09-15 11:00:13]

klaasb@google.com changed reviewers:
- danno@chromium.org

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

klaasb@google.com changed reviewers:
+ jarin@chromium.org

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

jarin@chromium.org: Please review deletions in crankshaft in particular.

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

Looks good overall, only real comment regards optional capacity parameter on
AllocateUninitializedJSArray

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

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5670: if (assembler->Is64()) {
On 2016/09/14 18:24:51, klaasb wrote:
> On 2016/09/13 20:29:59, rmcilroy wrote:
> > Any idea why this is different on 64bit?
> 
> This relates to the comment (now) at
> https://cs.chromium.org/chromium/src/v8/src/code-stub-assembler.h?l=53
> It's just passed on to ElementOffsetFromIndex.

Interesting. I wonder why it isn't faster to use SMIs on x64.

> We don't really need to untag the length, as we need it tagged to store it
> anyway and we don't do any arithmetic with it anyway. The current state was
the
> last attempt to make it a bit more consistent, but I've changed it to have the
> ParameterMode argument on AllocateUninitialized(/Empty)JSArray refer only to
> capacity.

Sounds good, thanks.

https://codereview.chromium.org/2304573004/diff/280001/src/code-stubs.cc#newc...
src/code-stubs.cc:5789: Heap::kEmptyFixedArrayRootIndex);
On 2016/09/14 18:24:51, klaasb wrote:
> On 2016/09/13 20:29:59, rmcilroy wrote:
> > Would it be worth merging the allocations for fixed cow and zero capacity
> (i.e.,
> > having length and the elements object be Variables set in the branches, then
> > merging to a common allocation branch which does the allocation?
> 
> I thought about that before, but seem to have forgotten to try it. It
certainly
> reduces code size and removes a load of a spilled value. Done.
> 
> Sharing more code at runtime for the other two branches is not easily done,
> however.

Yeah I realized the other branches couldn't be shared quite as easily. Thanks
for merging this bit.

https://codereview.chromium.org/2304573004/diff/300001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2304573004/diff/300001/src/code-stub-assemble...
src/code-stub-assembler.cc:1409: if (capacity != nullptr) {
Not a great fan of this being an optional nullptr and having to pass
ParameterMode capacity_mode too.How about just passing in elements_size or
total_size as a node and do the calculation outside this function?

https://codereview.chromium.org/2304573004/diff/300001/src/code-stub-assemble...
src/code-stub-assembler.cc:1438: return {array, nullptr};
Also not keen on this maybe returning a nullptr. Maybe just add another function
for allocating an array with elements (which calls this one).

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

https://codereview.chromium.org/2304573004/diff/300001/src/code-stubs.cc#newc...
src/code-stubs.cc:5752: Label correct_elements_map(assembler), abort(assembler);
kDeferred on abort label.

https://codereview.chromium.org/2304573004/diff/300001/src/code-stubs.cc#newc...
src/code-stubs.cc:5761:
assembler->SmiConstant(Smi::FromInt(BailoutReason::kUnexpectedMap));
This could be more specific (i.e., "expected fixed double array map")

https://codereview.chromium.org/2304573004/diff/300001/src/code-stubs.cc#newc...
src/code-stubs.cc:5766: Node* array = NonEmptyShallowClone(assembler,
boilerplate, boilerplate_map,
nit - newline above

[Jarin, 2016-09-16 16:19:36]

jarin@chromium.org changed required reviewers:
+ rmcilroy@chromium.org

[Jarin, 2016-09-16 16:19:37]

lgtm for the crankshaft bit.

[klaasb, 2016-09-19 12:18:00]

https://codereview.chromium.org/2304573004/diff/300001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2304573004/diff/300001/src/code-stub-assemble...
src/code-stub-assembler.cc:1409: if (capacity != nullptr) {
On 2016/09/16 08:53:51, rmcilroy wrote:
> Not a great fan of this being an optional nullptr and having to pass
> ParameterMode capacity_mode too.How about just passing in elements_size or
> total_size as a node and do the calculation outside this function?

Done.

https://codereview.chromium.org/2304573004/diff/300001/src/code-stub-assemble...
src/code-stub-assembler.cc:1438: return {array, nullptr};
On 2016/09/16 08:53:51, rmcilroy wrote:
> Also not keen on this maybe returning a nullptr. Maybe just add another
function
> for allocating an array with elements (which calls this one).

Done.

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

https://codereview.chromium.org/2304573004/diff/300001/src/code-stubs.cc#newc...
src/code-stubs.cc:5752: Label correct_elements_map(assembler), abort(assembler);
On 2016/09/16 08:53:52, rmcilroy wrote:
> kDeferred on abort label.

Done.

https://codereview.chromium.org/2304573004/diff/300001/src/code-stubs.cc#newc...
src/code-stubs.cc:5761:
assembler->SmiConstant(Smi::FromInt(BailoutReason::kUnexpectedMap));
On 2016/09/16 08:53:52, rmcilroy wrote:
> This could be more specific (i.e., "expected fixed double array map")

Done.

https://codereview.chromium.org/2304573004/diff/300001/src/code-stubs.cc#newc...
src/code-stubs.cc:5766: Node* array = NonEmptyShallowClone(assembler,
boilerplate, boilerplate_map,
On 2016/09/16 08:53:51, rmcilroy wrote:
> nit - newline above

Done.

[Michael Starzinger, 2016-09-19 14:20:38]

https://codereview.chromium.org/2304573004/diff/320001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2304573004/diff/320001/src/code-stub-assemble...
src/code-stub-assembler.h:335: ParameterMode capacity_mode =
INTEGER_PARAMETERS);
It is somewhat un-intuitive that the ParameterMode applies only to the capacity
but not to the length. AFAICT the only place this contract is encoded is in the
name of the argument. We should at least clearify that in the comment to this
function.

Alternatively we could pass in two ParameterMode arguments, each applying to
their respective node only.

https://codereview.chromium.org/2304573004/diff/320001/src/code-stub-assemble...
src/code-stub-assembler.h:336: // Allocate a JSArray and fill elements with
zero.
nit: s/zero/the hole/

[klaasb, 2016-09-19 14:46:37]

https://codereview.chromium.org/2304573004/diff/320001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2304573004/diff/320001/src/code-stub-assemble...
src/code-stub-assembler.h:335: ParameterMode capacity_mode =
INTEGER_PARAMETERS);
On 2016/09/19 14:20:38, Michael Starzinger wrote:
> It is somewhat un-intuitive that the ParameterMode applies only to the
capacity
> but not to the length. AFAICT the only place this contract is encoded is in
the
> name of the argument. We should at least clearify that in the comment to this
> function.

Well, it also doesn't apply to any of the other two Node* parameters, which is
also only kind of encoded in their names. ;)
I'll clarify. 

> Alternatively we could pass in two ParameterMode arguments, each applying to
> their respective node only.

The method is only using the length in one way. Doing this would suggest to me
that something significant should happen depending on the ParameterMode, but it
would really only delay the (potential) tagging until the use.
In general I don't really like specifying the types of parameters separately as
another parameter, but typing the Nodes might be a bit overkill and might
introduce compile time issues (templates...).

https://codereview.chromium.org/2304573004/diff/320001/src/code-stub-assemble...
src/code-stub-assembler.h:336: // Allocate a JSArray and fill elements with
zero.
On 2016/09/19 14:20:38, Michael Starzinger wrote:
> nit: s/zero/the hole/

Done, oops.

[rmcilroy, 2016-09-19 15:47:48]

LGTM, thanks.

https://codereview.chromium.org/2304573004/diff/340001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2304573004/diff/340001/src/code-stub-assemble...
src/code-stub-assembler.h:342: ParameterMode mode = INTEGER_PARAMETERS);
capacity_mode here too (with comment) ?

[klaasb, 2016-09-19 18:39:25]

https://codereview.chromium.org/2304573004/diff/340001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2304573004/diff/340001/src/code-stub-assemble...
src/code-stub-assembler.h:342: ParameterMode mode = INTEGER_PARAMETERS);
On 2016/09/19 15:47:48, rmcilroy wrote:
> capacity_mode here too (with comment) ?

Done.

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

https://codereview.chromium.org/2304573004/diff/380001/src/code-stubs.cc#newc...
src/code-stubs.cc:5815: CodeStubAssembler::SMI_PARAMETERS);
This live range breaking allows the TF instruction selection to find that it can
use this computation directly in an addressing mode for CMP and doesn't need to
put it into a register for the jump above.

Frame is still there on ia32, maybe there's more I can do with breaking live
ranges.
TF is still putting the undefined (and other pointer constants used with CMPs)
in a register because of https://bugs.chromium.org/p/v8/issues/detail?id=5392

[Michael Starzinger, 2016-09-20 09:01:17]

LGTM.

[klaasb, 2016-09-20 12:04:44]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-09-20 12:04:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2304573004/#ps380001
(title: "break live range of allocation_site so instruction selection uses nice
addressing modes")

[commit-bot: I haz the power, 2016-09-20 12:04:53]

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 12:35:42]

Description was changed from

==========
Port FastCloneShallowArrayStub to Turbofan

Refactors CodeStubAssembler::AllocateJSArray to share code.

BUG=chromium:608675
==========

to

==========
Port FastCloneShallowArrayStub to Turbofan

Refactors CodeStubAssembler::AllocateJSArray to share code.

BUG=chromium:608675
==========

[commit-bot: I haz the power, 2016-09-20 12:35:43]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-09-20 12:36:38]

Description was changed from

==========
Port FastCloneShallowArrayStub to Turbofan

Refactors CodeStubAssembler::AllocateJSArray to share code.

BUG=chromium:608675
==========

to

==========
Port FastCloneShallowArrayStub to Turbofan

Refactors CodeStubAssembler::AllocateJSArray to share code.

BUG=chromium:608675

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

[commit-bot: I haz the power, 2016-09-20 12:36:40]

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