[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

Improves performance in simple, single element case by 5% and in multiple
elements cases by 2%.

BUG=chromium:608675
LOG=N

Committed: https://crrev.com/df2578d2ec630ccd08619e1f733da3f61e7ee380
Cr-Commit-Position: refs/heads/master@{#41368}

[danno, 2016-11-16 12:34:01]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 12:34:11]

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-11-16 12:35:41]

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

[commit-bot: I haz the power, 2016-11-16 12:35:42]

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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28665)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/17853)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/17963)

[danno, 2016-11-16 12:36:03]

Description was changed from

==========
Start porting ArrayPush

BUG=
==========

to

==========
[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

BUG=chromium:608675
LOG=N
==========

[danno, 2016-11-16 14:32:15]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 14:32:20]

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-11-16 14:43:49]

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

[commit-bot: I haz the power, 2016-11-16 14:43:49]

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...)

[danno, 2016-11-16 15:24:31]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 15:24:40]

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-11-16 15:39:02]

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

[commit-bot: I haz the power, 2016-11-16 15:39:03]

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...)

[danno, 2016-11-16 17:13:03]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 17:13:15]

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-11-16 17:22:04]

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

[commit-bot: I haz the power, 2016-11-16 17:22:06]

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...)

[danno, 2016-11-16 17:54:03]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 17:54: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-11-16 18:24:52]

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

[commit-bot: I haz the power, 2016-11-16 18:24:53]

Dry run: This issue passed the CQ dry run.

[danno, 2016-11-17 09:43:46]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 09:43:57]

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-11-17 10:11:37]

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

[commit-bot: I haz the power, 2016-11-17 10:11:38]

Dry run: This issue passed the CQ dry run.

[danno, 2016-11-17 17:33:23]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 17:33:29]

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-11-17 18:05:11]

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

[commit-bot: I haz the power, 2016-11-17 18:05:12]

Dry run: This issue passed the CQ dry run.

[danno, 2016-11-18 13:11:57]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 13:12:01]

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

[danno, 2016-11-18 13:15:54]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 13:16:01]

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

[danno, 2016-11-18 13:16:38]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 13:16:41]

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

[danno, 2016-11-18 13:43:01]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 13:43:07]

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-11-18 14:10:55]

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

[commit-bot: I haz the power, 2016-11-18 14:10:56]

Dry run: This issue passed the CQ dry run.

[danno, 2016-11-18 15:08:22]

danno@chromium.org changed reviewers:
+ ishell@google.com, jkummerow@chromium.org

[danno, 2016-11-18 15:08:23]

PTAL

[Jakob Kummerow, 2016-11-23 17:17:06]

Looks good! Mostly nits.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:199: {
nit: this identation level doesn't add anything (except whitespace). I'd just
drop it; if you want to keep it, then at least you should pull the
"Bind(&runtime)" section out of it.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:204: Node* bit_field2 =
assembler.LoadObjectField(map, Map::kBitField2Offset,
... = assembler.LoadMapBitField2(map);

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:218: Node* bit_field3 =
assembler.LoadObjectField(map, Map::kBitField3Offset,
... = assembler.LoadMapBitField3(map);

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:223: assembler.GotoIf(assembler.Word32Equal(test,
mask_node), &runtime);
assembler.GotoIf(assembler.IsSetWord32<Map::DictionaryMap>(bit_field3),
&runtime);

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:225: // Check whether the length property is
writable. The length property is the
Most excellent point. The KeyedStoreGeneric stub has never done this. I've filed
https://bugs.chromium.org/p/v8/issues/detail?id=5669.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:228: Node* descriptors =
assembler.LoadObjectField(map, Map::kDescriptorsOffset);
... = assembler.LoadMapDescriptors(map);

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:233: mask_node =
assembler.SmiConstant(Smi::FromInt(mask));
nit: s/Smi::FromInt(mask)/mask/

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:241: Label default_label(&assembler, vars);
nit: why not just s/vars/&arg_index/ here and once more in line 244?

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:272: arg,
assembler.SmiConstant(Smi::FromInt(STRICT)));
nit: s/Smi::FromInt(STRICT)/STRICT/

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:310: arg,
assembler.SmiConstant(Smi::FromInt(STRICT)));
nit: s/Smi::FromInt(STRICT)/STRICT/

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:325:
assembler->SmiConstant(Smi::FromInt(STRICT)));
nit: s/Smi::FromInt(STRICT)/STRICT/

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

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:1387: capacity.Bind(
I think |capacity| could simply be a Node*, I see no reason why it needs to be a
Variable.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:1411: [kind, array, context, mode, &length,
&elements, &pre_bailout](
nit: |array| and |context| appear to be unused, drop them

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:1414:
assembler->GotoUnless(assembler->TaggedIsSmi(arg), &pre_bailout);
nit: now that you have TaggedIsNotSmi, you could use it here

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:2216: INTPTR_PARAMETERS);
nit: INTPTR_PARAMETERS is the default, no need to specify it explicitly

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:2644: LoadObjectField(value,
HeapNumber::kValueOffset, MachineType::Float64()));
This doesn't canonicalize the hole NaN. Use Float64SilenceNaN(Node*
double_value).

You can use a variant of mjsunit/compiler/regress-store-holey-double-array.js to
test for this case.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:9027: Branch(Word32NotEqual(input_map,
HeapNumberMapConstant()), is_not_number,
Maps are words, not word32s! Even better:

Branch(IsHeapNumberMap(input_map), ...)

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:9035: GotoIf(Word32Equal(input_map,
HeapNumberMapConstant()), is_number);
Same here

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:9131:
CodeStubAssembler::FastJSArrayAccessMode::INBOUNDS_READ, &if_isfast,
nit: s/CodeStubAssembler:://

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

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.h:152: void GotoIfNotNumber(compiler::Node* value,
Label* is_not_number);
nit: when you rebase, please drop all the "compiler::" prefixes from your
changes in this file, we don't need them anymore.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
File test/cctest/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2006:
TEST(BuildAppendJSArrayFastElement) {
High-level comment: We certainly shouldn't over-engineer our tests, but these
here are *very* repetitive. How about pulling out the (entire!) body as a helper
function, parameterized on elements kind and initial capacity? That should cover
4 out of 5 tests below. It's probably easiest to always pass FAST_SMI_ELEMENTS
to NewJSArray(), and then unconditionally do a
JSObject::TransitionElementsKind(array, elements_kind) like the double test
already does (it'll simply bail out for attempted smi->smi transitions).

If you want to go a step further and cover all five tests with the helper
function, parameterize on expected_final_array_length too, and do:

ft.Call(...,
        expected_final_array_length > 4 ? Handle<Smi>(...) : undefined
        ...,

and update the final sequence of CHECK_EQs. But test readability might suffer
from that.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2017:
(void)JSObject::SetElement(isolate, array, 0,
Instead of "(void)JSObject::SetElement(...)", do
"JSObject::SetElement(...).Check()". Again 9 more times below.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2065: CodeAssemblerTester data(isolate,
0);
don't you mean s/0/kNumParams/? How does this even work with 0?

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2118: CodeAssemblerTester data(isolate,
0);
same here

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2172: CodeAssemblerTester data(isolate,
0);
same here

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2222: CodeAssemblerTester data(isolate,
0);
same here

https://codereview.chromium.org/2497243002/diff/260001/test/mjsunit/array-pus...
File test/mjsunit/array-push13.js (right):

https://codereview.chromium.org/2497243002/diff/260001/test/mjsunit/array-pus...
test/mjsunit/array-push13.js:6: var __v_10 = 0;
nit: unused variable

https://codereview.chromium.org/2497243002/diff/260001/test/mjsunit/array-pus...
test/mjsunit/array-push13.js:13: __v_9.push(undefined);
nit: I'd push some recognizable sentinel. Maybe |true|, or some string. Don't
forget to update the expectation below.

[danno, 2016-11-28 22:34:35]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 22:34: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-11-28 23:22:33]

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

[commit-bot: I haz the power, 2016-11-28 23:22:34]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16929)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[danno, 2016-11-29 11:04:09]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 11:04:17]

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-11-29 11:44:41]

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

[commit-bot: I haz the power, 2016-11-29 11:44:42]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16967)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[danno, 2016-11-29 14:39:20]

Description was changed from

==========
[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

BUG=chromium:608675
LOG=N
==========

to

==========
[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

Improves performance in simple, single element case by 5% and in multiple
elements cases by 2%.

BUG=chromium:608675
LOG=N
==========

[danno, 2016-11-29 14:40:00]

Please take another look

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:199: {
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> nit: this identation level doesn't add anything (except whitespace). I'd just
> drop it; if you want to keep it, then at least you should pull the
> "Bind(&runtime)" section out of it.

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:204: Node* bit_field2 =
assembler.LoadObjectField(map, Map::kBitField2Offset,
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> ... = assembler.LoadMapBitField2(map);

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:218: Node* bit_field3 =
assembler.LoadObjectField(map, Map::kBitField3Offset,
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> ... = assembler.LoadMapBitField3(map);

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:225: // Check whether the length property is
writable. The length property is the
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> Most excellent point. The KeyedStoreGeneric stub has never done this. I've
filed
> https://bugs.chromium.org/p/v8/issues/detail?id=5669.

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:228: Node* descriptors =
assembler.LoadObjectField(map, Map::kDescriptorsOffset);
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> ... = assembler.LoadMapDescriptors(map);

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:233: mask_node =
assembler.SmiConstant(Smi::FromInt(mask));
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> nit: s/Smi::FromInt(mask)/mask/

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:241: Label default_label(&assembler, vars);
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> nit: why not just s/vars/&arg_index/ here and once more in line 244?

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:272: arg,
assembler.SmiConstant(Smi::FromInt(STRICT)));
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> nit: s/Smi::FromInt(STRICT)/STRICT/

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:310: arg,
assembler.SmiConstant(Smi::FromInt(STRICT)));
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> nit: s/Smi::FromInt(STRICT)/STRICT/

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/builtins/builtins-...
src/builtins/builtins-array.cc:325:
assembler->SmiConstant(Smi::FromInt(STRICT)));
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> nit: s/Smi::FromInt(STRICT)/STRICT/

Done.

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

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:1387: capacity.Bind(
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> I think |capacity| could simply be a Node*, I see no reason why it needs to be
a
> Variable.

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:1411: [kind, array, context, mode, &length,
&elements, &pre_bailout](
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> nit: |array| and |context| appear to be unused, drop them

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:1414:
assembler->GotoUnless(assembler->TaggedIsSmi(arg), &pre_bailout);
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> nit: now that you have TaggedIsNotSmi, you could use it here

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:2216: INTPTR_PARAMETERS);
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> nit: INTPTR_PARAMETERS is the default, no need to specify it explicitly

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:2644: LoadObjectField(value,
HeapNumber::kValueOffset, MachineType::Float64()));
On 2016/11/23 17:17:05, Jakob Kummerow wrote:
> This doesn't canonicalize the hole NaN. Use Float64SilenceNaN(Node*
> double_value).
> 
> You can use a variant of mjsunit/compiler/regress-store-holey-double-array.js
to
> test for this case.

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:9027: Branch(Word32NotEqual(input_map,
HeapNumberMapConstant()), is_not_number,
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> Maps are words, not word32s! Even better:
> 
> Branch(IsHeapNumberMap(input_map), ...)

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:9035: GotoIf(Word32Equal(input_map,
HeapNumberMapConstant()), is_number);
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> Same here

Done.

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.cc:9131:
CodeStubAssembler::FastJSArrayAccessMode::INBOUNDS_READ, &if_isfast,
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> nit: s/CodeStubAssembler:://

Done.

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

https://codereview.chromium.org/2497243002/diff/260001/src/code-stub-assemble...
src/code-stub-assembler.h:152: void GotoIfNotNumber(compiler::Node* value,
Label* is_not_number);
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> nit: when you rebase, please drop all the "compiler::" prefixes from your
> changes in this file, we don't need them anymore.

Done.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
File test/cctest/test-code-stub-assembler.cc (right):

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2006:
TEST(BuildAppendJSArrayFastElement) {
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> High-level comment: We certainly shouldn't over-engineer our tests, but these
> here are *very* repetitive. How about pulling out the (entire!) body as a
helper
> function, parameterized on elements kind and initial capacity? That should
cover
> 4 out of 5 tests below. It's probably easiest to always pass FAST_SMI_ELEMENTS
> to NewJSArray(), and then unconditionally do a
> JSObject::TransitionElementsKind(array, elements_kind) like the double test
> already does (it'll simply bail out for attempted smi->smi transitions).
> 
> If you want to go a step further and cover all five tests with the helper
> function, parameterize on expected_final_array_length too, and do:
> 
> ft.Call(...,
>         expected_final_array_length > 4 ? Handle<Smi>(...) : undefined
>         ...,
> 
> and update the final sequence of CHECK_EQs. But test readability might suffer
> from that.

Done.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2017:
(void)JSObject::SetElement(isolate, array, 0,
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> Instead of "(void)JSObject::SetElement(...)", do
> "JSObject::SetElement(...).Check()". Again 9 more times below.

Done.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2065: CodeAssemblerTester data(isolate,
0);
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> don't you mean s/0/kNumParams/? How does this even work with 0?

Done.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2118: CodeAssemblerTester data(isolate,
0);
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> same here

Done.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2172: CodeAssemblerTester data(isolate,
0);
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> same here

Done.

https://codereview.chromium.org/2497243002/diff/260001/test/cctest/test-code-...
test/cctest/test-code-stub-assembler.cc:2222: CodeAssemblerTester data(isolate,
0);
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> same here

Done.

https://codereview.chromium.org/2497243002/diff/260001/test/mjsunit/array-pus...
File test/mjsunit/array-push13.js (right):

https://codereview.chromium.org/2497243002/diff/260001/test/mjsunit/array-pus...
test/mjsunit/array-push13.js:6: var __v_10 = 0;
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> nit: unused variable

Done.

https://codereview.chromium.org/2497243002/diff/260001/test/mjsunit/array-pus...
test/mjsunit/array-push13.js:13: __v_9.push(undefined);
On 2016/11/23 17:17:06, Jakob Kummerow wrote:
> nit: I'd push some recognizable sentinel. Maybe |true|, or some string. Don't
> forget to update the expectation below.

Done.

[Jakob Kummerow, 2016-11-29 15:06:03]

lgtm

[danno, 2016-11-29 15:09:01]

The CQ bit was checked by danno@chromium.org

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

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

[danno, 2016-11-29 15:09:34]

The CQ bit was unchecked by danno@chromium.org

[danno, 2016-11-29 15:33:59]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 15:34:03]

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-11-29 16:07:34]

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

[commit-bot: I haz the power, 2016-11-29 16:07:35]

Dry run: This issue passed the CQ dry run.

[danno, 2016-11-29 16:56:14]

The CQ bit was checked by danno@chromium.org

[danno, 2016-11-29 16:56:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/2497243002/#ps340001
(title: "Fix GC mole")

[commit-bot: I haz the power, 2016-11-29 16:56:20]

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-11-29 16:57:59]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1480438574567640,
"parent_rev": "0db306a536490f9ad3b82702cf7e6e3028e28936", "commit_rev":
"fde0caa9aa299f1e941839d766afce925765e84d"}

[commit-bot: I haz the power, 2016-11-29 16:58:03]

Description was changed from

==========
[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

Improves performance in simple, single element case by 5% and in multiple
elements cases by 2%.

BUG=chromium:608675
LOG=N
==========

to

==========
[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

Improves performance in simple, single element case by 5% and in multiple
elements cases by 2%.

BUG=chromium:608675
LOG=N
==========

[commit-bot: I haz the power, 2016-11-29 16:58:05]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-11-29 16:58:42]

Description was changed from

==========
[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

Improves performance in simple, single element case by 5% and in multiple
elements cases by 2%.

BUG=chromium:608675
LOG=N
==========

to

==========
[stubs] Port builtin for Array.push fast-case from Crankshaft to TF

Improves performance in simple, single element case by 5% and in multiple
elements cases by 2%.

BUG=chromium:608675
LOG=N

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

[commit-bot: I haz the power, 2016-11-29 16:58:43]

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