[stubs] Generalize loop handling in CodeStubAssembler and improve common loop performance

[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop performance

Specifically an attempt to address a 3.5% regression on the total load
time on cnn introduced by https://codereview.chromium.org/2113673002.

Non-refactoring effect of this CL is to reduce the number of branches in
CodeStubAssembler-generated loops iterating over FixedArrays from
two to one.

LOG=N
BUG=v8:5423
Committed: https://crrev.com/697aa6f579675db873fdc4c2eaf578fab8126b40
Cr-Commit-Position: refs/heads/master@{#40020}

[danno, 2016-09-29 08:58:05]

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

[commit-bot: I haz the power, 2016-09-29 08:58: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-09-29 09:21:23]

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

[commit-bot: I haz the power, 2016-09-29 09:21:24]

Dry run: Try jobs failed on following builders:
  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_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[danno, 2016-09-29 12:58:07]

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

[commit-bot: I haz the power, 2016-09-29 12:58:13]

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-29 13:35:41]

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

[commit-bot: I haz the power, 2016-09-29 13:35:42]

Dry run: This issue passed the CQ dry run.

[danno, 2016-09-30 13:14:44]

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

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

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-30 13:17:48]

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

[commit-bot: I haz the power, 2016-09-30 13:17:48]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/13805)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/13771)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/9996)

[danno, 2016-09-30 13:28:20]

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

[commit-bot: I haz the power, 2016-09-30 13:28:25]

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-30 13:52:49]

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

[commit-bot: I haz the power, 2016-09-30 13:52:50]

Dry run: This issue passed the CQ dry run.

[danno, 2016-09-30 14:20:40]

Description was changed from

==========
[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop
perforamnce

LOG=N
BUG=
==========

to

==========
[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop
perforamnce

Specifically an attempt to address a 3.5% regression on the total load
time on cnn introduced by https://codereview.chromium.org/2113673002. 

Non-refactoring effect of this CL is to reduce the number of branches in
CodeStubAssembler-generated loops iterating over FixedArrays from 
two to one.

LOG=N
BUG=v8:5423
==========

[danno, 2016-09-30 14:20:40]

danno@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org

[danno, 2016-09-30 14:25:00]

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

[commit-bot: I haz the power, 2016-09-30 14:25: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-30 14:51:36]

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

[commit-bot: I haz the power, 2016-09-30 14:51:37]

Dry run: This issue passed the CQ dry run.

[danno, 2016-09-30 15:53:23]

ptal

[Benedikt Meurer, 2016-09-30 17:15:29]

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

[Benedikt Meurer, 2016-09-30 17:15:30]

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

[Benedikt Meurer, 2016-09-30 17:15:30]

Definitely something Igor should review. In general Igor is considered the
(main) owner of CodeStubAssembler now and all non-trivial changes should have an
LGTM from Igor.

This is LGTM from my side (although I wish we could make this less fancy...
higher order functions in C++).

[Igor Sheludko, 2016-10-04 08:54:08]

Description was changed from

==========
[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop
perforamnce

Specifically an attempt to address a 3.5% regression on the total load
time on cnn introduced by https://codereview.chromium.org/2113673002. 

Non-refactoring effect of this CL is to reduce the number of branches in
CodeStubAssembler-generated loops iterating over FixedArrays from 
two to one.

LOG=N
BUG=v8:5423
==========

to

==========
[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop
performance

Specifically an attempt to address a 3.5% regression on the total load
time on cnn introduced by https://codereview.chromium.org/2113673002. 

Non-refactoring effect of this CL is to reduce the number of branches in
CodeStubAssembler-generated loops iterating over FixedArrays from 
two to one.

LOG=N
BUG=v8:5423
==========

[Igor Sheludko, 2016-10-04 09:32:25]

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

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5661: {
Why not just put 
  GotoIf(WordEqual(var.value(), end_index), &after_loop);
here instead of a condition check before the loop and another one below?
Is it about how TurboFan orders the blocks?

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5681: ParameterMode mode, ForEachDirection direction)
{
I think we should put this assert somewhere in this function:

// The code below relies on array headers having the same size.
STATIC_ASSERT(FixedArray::kHeaderSize == FixedDoubleArray::kHeaderSize);

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5694: ElementOffsetFromIndex(index, FAST_ELEMENTS,
mode,
I think you intended to use |kind| instead of |FAST_ELEMENTS| and 
INTPTR_PARAMETERS instead of |mode| here.

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5702: ElementOffsetFromIndex(index, FAST_ELEMENTS,
mode,
Same here

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5722: [fixed_array, body](CodeStubAssembler*
assembler, Node* index) {
s/index/offset/

[danno, 2016-10-04 16:09:32]

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

[commit-bot: I haz the power, 2016-10-04 16:09:37]

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

[danno, 2016-10-04 16:10:23]

Feedback addressed. PTAL.

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

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5661: {
On 2016/10/04 09:32:25, Igor Sheludko wrote:
> Why not just put 
>   GotoIf(WordEqual(var.value(), end_index), &after_loop);
> here instead of a condition check before the loop and another one below?
> Is it about how TurboFan orders the blocks?

As discussed offline, for efficiency. Comment added.

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5681: ParameterMode mode, ForEachDirection direction)
{
On 2016/10/04 09:32:25, Igor Sheludko wrote:
> I think we should put this assert somewhere in this function:
> 
> // The code below relies on array headers having the same size.
> STATIC_ASSERT(FixedArray::kHeaderSize == FixedDoubleArray::kHeaderSize);

Done.

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5694: ElementOffsetFromIndex(index, FAST_ELEMENTS,
mode,
On 2016/10/04 09:32:25, Igor Sheludko wrote:
> I think you intended to use |kind| instead of |FAST_ELEMENTS| and 
> INTPTR_PARAMETERS instead of |mode| here.

Done.

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5702: ElementOffsetFromIndex(index, FAST_ELEMENTS,
mode,
On 2016/10/04 09:32:25, Igor Sheludko wrote:
> Same here

Done.

https://codereview.chromium.org/2380953002/diff/100001/src/code-stub-assemble...
src/code-stub-assembler.cc:5722: [fixed_array, body](CodeStubAssembler*
assembler, Node* index) {
On 2016/10/04 09:32:25, Igor Sheludko wrote:
> s/index/offset/

Done.

[commit-bot: I haz the power, 2016-10-04 16:10:47]

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

[commit-bot: I haz the power, 2016-10-04 16:10:48]

Dry run: Try jobs failed on following builders:
  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_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10100)

[danno, 2016-10-04 16:13:43]

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

[commit-bot: I haz the power, 2016-10-04 16:13:47]

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-10-04 16:43:17]

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

[commit-bot: I haz the power, 2016-10-04 16:43:18]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-10-05 07:33:19]

lgtm with a nit:

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

https://codereview.chromium.org/2380953002/diff/140001/src/code-stub-assemble...
src/code-stub-assembler.cc:5704: // loop that help's turbofan generate better
code. If there's only a single
s/help's/helps/

[danno, 2016-10-05 16:39:07]

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

[commit-bot: I haz the power, 2016-10-05 16:39:12]

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

[danno, 2016-10-05 16:57:51]

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

[commit-bot: I haz the power, 2016-10-05 16:58:02]

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-10-05 17:10:54]

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

[commit-bot: I haz the power, 2016-10-05 17:10:55]

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

[danno, 2016-10-05 20:51:10]

Patchset #12 (id:220001) has been deleted

[danno, 2016-10-05 20:54:08]

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

[commit-bot: I haz the power, 2016-10-05 20:54: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-10-05 21:22:41]

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

[commit-bot: I haz the power, 2016-10-05 21:22:42]

Dry run: This issue passed the CQ dry run.

[danno, 2016-10-06 06:36:38]

The CQ bit was checked by danno@chromium.org

[danno, 2016-10-06 06:36:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2380953002/#ps240001
(title: "Working version")

[commit-bot: I haz the power, 2016-10-06 06:36:50]

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

Description was changed from

==========
[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop
performance

Specifically an attempt to address a 3.5% regression on the total load
time on cnn introduced by https://codereview.chromium.org/2113673002. 

Non-refactoring effect of this CL is to reduce the number of branches in
CodeStubAssembler-generated loops iterating over FixedArrays from 
two to one.

LOG=N
BUG=v8:5423
==========

to

==========
[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop
performance

Specifically an attempt to address a 3.5% regression on the total load
time on cnn introduced by https://codereview.chromium.org/2113673002. 

Non-refactoring effect of this CL is to reduce the number of branches in
CodeStubAssembler-generated loops iterating over FixedArrays from 
two to one.

LOG=N
BUG=v8:5423
==========

[commit-bot: I haz the power, 2016-10-06 06:40:11]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-10-06 06:40:28]

Description was changed from

==========
[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop
performance

Specifically an attempt to address a 3.5% regression on the total load
time on cnn introduced by https://codereview.chromium.org/2113673002. 

Non-refactoring effect of this CL is to reduce the number of branches in
CodeStubAssembler-generated loops iterating over FixedArrays from 
two to one.

LOG=N
BUG=v8:5423
==========

to

==========
[stubs]: Generalize loop handling in CodeStubAssembler and improve common loop
performance

Specifically an attempt to address a 3.5% regression on the total load
time on cnn introduced by https://codereview.chromium.org/2113673002.

Non-refactoring effect of this CL is to reduce the number of branches in
CodeStubAssembler-generated loops iterating over FixedArrays from
two to one.

LOG=N
BUG=v8:5423

Committed: https://crrev.com/697aa6f579675db873fdc4c2eaf578fab8126b40
Cr-Commit-Position: refs/heads/master@{#40020}
==========

[commit-bot: I haz the power, 2016-10-06 06:40:30]

Patchset 12 (id:??) landed as
https://crrev.com/697aa6f579675db873fdc4c2eaf578fab8126b40
Cr-Commit-Position: refs/heads/master@{#40020}