[WASM SIMD] Implement horizontal add for float and integer types.

[WASM SIMD] Implement horizontal add for float and integer types.
- Adds new F32x4AddHoriz, I32x4AddHoriz, etc. to WASM opcodes.
- Implements them for ARM.

LOG=N
BUG=v8:6020
Review-Url: https://codereview.chromium.org/2804883008
Cr-Commit-Position: refs/heads/master@{#44812}
Committed: https://chromium.googlesource.com/v8/v8/+/a71c338d9e24e55b47125108a0fce754076751d0

[bbudge, 2017-04-19 22:01:21]

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

[commit-bot: I haz the power, 2017-04-19 22:01: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, 2017-04-19 22:04:47]

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

[commit-bot: I haz the power, 2017-04-19 22:04:49]

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

[bbudge, 2017-04-19 22:18:11]

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

[commit-bot: I haz the power, 2017-04-19 22:18:18]

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

[bbudge, 2017-04-19 22:19:02]

bbudge@chromium.org changed reviewers:
+ bmeurer@chromium.org, martyn.capewell@arm.com

[commit-bot: I haz the power, 2017-04-19 22:21:47]

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

[commit-bot: I haz the power, 2017-04-19 22:21:48]

Dry run: Try jobs failed on following builders:
  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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  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/24586)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/21177)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)

[bbudge, 2017-04-19 23:40:19]

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

[commit-bot: I haz the power, 2017-04-19 23:40:28]

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, 2017-04-19 23:44:58]

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

[commit-bot: I haz the power, 2017-04-19 23:44:59]

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

[bbudge, 2017-04-20 00:03:07]

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

[commit-bot: I haz the power, 2017-04-20 00:03: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, 2017-04-20 00:30:26]

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

[commit-bot: I haz the power, 2017-04-20 00:30:30]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2017-04-20 08:23:18]

bmeurer@chromium.org changed required reviewers:
+ martyn.capewell@arm.com

[Benedikt Meurer, 2017-04-20 08:23:18]

LGTM from my side.

[georgia.kouveli, 2017-04-20 14:53:05]

georgia.kouveli@arm.com changed reviewers:
+ georgia.kouveli@arm.com

[georgia.kouveli, 2017-04-20 14:53:06]

I was wondering whether there is a (WIP?) specification for these operations, as
I found the naming "horizontal add" a bit unexpected (see comments below).

That would also help with some details, like what happens when there is overflow
in the integer case (is it well-defined)?

https://codereview.chromium.org/2804883008/diff/100001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/2804883008/diff/100001/src/arm/assembler-arm....
src/arm/assembler-arm.h:80: V(d24) V(d25) V(d26) V(d27) V(d28) V(d29)
Why make d30 and d31 non-allocatable?

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/arm/code-...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/arm/code-...
src/compiler/arm/code-generator-arm.cc:1620: __ vpadd(dst.low(), src0.low(),
src0.high());
The inputs to all the vpadd instructions seem to be in the wrong order,
shouldn't it be high first and low second? (VPADD is not commutative).

The RunF32x4HorizOpTest and other tests will not catch this, as the two inputs
have splatted values, which means the two pairs in dst.high() will have the same
result and the two pairs in dst.low() will also have the same result. It would
help to update the test to have a different expected value in each pair, this
way you would catch any such issues.

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/arm/code-...
src/compiler/arm/code-generator-arm.cc:1734: // Make sure we don't overwrite
source data before it's used.
Might make sense to factor out this code, since it's repeated for all types.

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/machine-o...
File src/compiler/machine-operator.cc (right):

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/machine-o...
src/compiler/machine-operator.cc:236: V(F32x4AddHoriz, Operator::kCommutative,
2, 0, 1)                       \
These nodes as implemented should not be marked commutative - if you try
changing the order of the operands in RunF32x4HorizOpTest, for example, the test
fails.

https://codereview.chromium.org/2804883008/diff/100001/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/2804883008/diff/100001/src/wasm/wasm-opcodes....
src/wasm/wasm-opcodes.h:293: V(F32x4AddHoriz, 0xe5b9, s_ss)         \
Perhaps pairwise add would be a better name than horizontal add? (I would think
horizontal add refers to adding up all the elements in a single vector).

[bbudge, 2017-04-21 20:18:45]

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

[commit-bot: I haz the power, 2017-04-21 20:18:55]

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

[bbudge, 2017-04-21 20:18:58]

Thanks for the thorough review!

https://codereview.chromium.org/2804883008/diff/100001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/2804883008/diff/100001/src/arm/assembler-arm....
src/arm/assembler-arm.h:80: V(d24) V(d25) V(d26) V(d27) V(d28) V(d29)
On 2017/04/20 14:53:06, georgia.kouveli wrote:
> Why make d30 and d31 non-allocatable?

That's my mistake, from a local merge. I'll restore those.

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/arm/code-...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/arm/code-...
src/compiler/arm/code-generator-arm.cc:1620: __ vpadd(dst.low(), src0.low(),
src0.high());
On 2017/04/20 14:53:06, georgia.kouveli wrote:
> The inputs to all the vpadd instructions seem to be in the wrong order,
> shouldn't it be high first and low second? (VPADD is not commutative).
> 
> The RunF32x4HorizOpTest and other tests will not catch this, as the two inputs
> have splatted values, which means the two pairs in dst.high() will have the
same
> result and the two pairs in dst.low() will also have the same result. It would
> help to update the test to have a different expected value in each pair, this
> way you would catch any such issues.

I changed the tests to use the permutation test machinery, which assigns unique
(integral) values to each lane of each vector. This eliminates a lot of code and
results in better tests (no more splatted inputs.)

I think there's some confusion in the assembler calls - vpadd stores the first
source operand as Vn, and the second source operand as Vm, which is flipped from
the normal assembler syntax. I'm not sure what to do about that though, as this
matches a lot of existing NEON assembler code. Perhaps another CL to straighten
out the confusion?

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/arm/code-...
src/compiler/arm/code-generator-arm.cc:1734: // Make sure we don't overwrite
source data before it's used.
On 2017/04/20 14:53:06, georgia.kouveli wrote:
> Might make sense to factor out this code, since it's repeated for all types.

Done. (And I did the same for the repetitious narrowing integer conversions
too.)

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/machine-o...
File src/compiler/machine-operator.cc (right):

https://codereview.chromium.org/2804883008/diff/100001/src/compiler/machine-o...
src/compiler/machine-operator.cc:236: V(F32x4AddHoriz, Operator::kCommutative,
2, 0, 1)                       \
On 2017/04/20 14:53:06, georgia.kouveli wrote:
> These nodes as implemented should not be marked commutative - if you try
> changing the order of the operands in RunF32x4HorizOpTest, for example, the
test
> fails.

Good catch, done.

https://codereview.chromium.org/2804883008/diff/100001/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/2804883008/diff/100001/src/wasm/wasm-opcodes....
src/wasm/wasm-opcodes.h:293: V(F32x4AddHoriz, 0xe5b9, s_ss)         \
On 2017/04/20 14:53:06, georgia.kouveli wrote:
> Perhaps pairwise add would be a better name than horizontal add? (I would
think
> horizontal add refers to adding up all the elements in a single vector).

Intel and MIPS naming is horizontal, though they are really only pairwise like
ARM.

MIPS - hadd, hsub
Intel - PHADDS  (packed horizontal add)

It's challenging to design a portable operation for this, since the MIPS
operation widens the sum (to avoid overflow) and there is no FP variant. Intel
and ARM have the normal behavior here, which is to return the result modulo 2 ^
number of bits per lane. Intel has no type for byte-sized lanes, and it seems
less useful than the others, so I'm removing that operation from this CL.

[commit-bot: I haz the power, 2017-04-21 20:49:41]

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

[commit-bot: I haz the power, 2017-04-21 20:49:42]

Dry run: This issue passed the CQ dry run.

[georgia.kouveli, 2017-04-24 12:42:21]

LGTM!

On 2017/04/21 20:18:58, bbudge wrote:
> 
> I think there's some confusion in the assembler calls - vpadd stores the first
> source operand as Vn, and the second source operand as Vm, which is flipped
from
> the normal assembler syntax. I'm not sure what to do about that though, as
this
> matches a lot of existing NEON assembler code. Perhaps another CL to
straighten
> out the confusion?
> 

I agree, the code actually is correct - the order of arguments in the assembler
is as it should be (dn first, then dm). The disassembly is correct for vpadd -
the operands in the simulator functions are in the opposite order (dm, dn) than
expected compared to the assembler functions and the disassembly, but they are
used correctly, so there doesn't seem to be any issue. Your improved tests are
very useful here!

[bbudge, 2017-04-24 18:10:20]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2017-04-24 18:10:20]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2804883008/#ps140001
(title: "Use permutation "test-pattern" machinery to properly test pairwise
ops.")

[commit-bot: I haz the power, 2017-04-24 18:10:30]

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, 2017-04-24 18:10:31]

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

[commit-bot: I haz the power, 2017-04-24 18:10:32]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[bbudge, 2017-04-24 18:11:52]

On 2017/04/24 12:42:21, georgia.kouveli wrote:
> LGTM!
> 
> On 2017/04/21 20:18:58, bbudge wrote:
> > 
> > I think there's some confusion in the assembler calls - vpadd stores the
first
> > source operand as Vn, and the second source operand as Vm, which is flipped
> from
> > the normal assembler syntax. I'm not sure what to do about that though, as
> this
> > matches a lot of existing NEON assembler code. Perhaps another CL to
> straighten
> > out the confusion?
> > 
> 
> I agree, the code actually is correct - the order of arguments in the
assembler
> is as it should be (dn first, then dm). The disassembly is correct for vpadd -
> the operands in the simulator functions are in the opposite order (dm, dn)
than
> expected compared to the assembler functions and the disassembly, but they are
> used correctly, so there doesn't seem to be any issue. Your improved tests are
> very useful here!

I ran the tests on actual ARM hardware just to make sure. Thanks again for your
reviews.

[bbudge, 2017-04-24 18:15:53]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2017-04-24 18:16:04]

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, 2017-04-24 18:16:05]

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

[commit-bot: I haz the power, 2017-04-24 18:16:06]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[bbudge, 2017-04-24 18:19:08]

bbudge@chromium.org changed reviewers:
- martyn.capewell@arm.com

[bbudge, 2017-04-24 18:19:09]

bbudge@chromium.org changed required reviewers:
- martyn.capewell@arm.com

[bbudge, 2017-04-24 18:19:10]

- Martyn, since Georgia reviewed.

[bbudge, 2017-04-24 18:19:18]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2017-04-24 18:19:32]

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, 2017-04-24 18:21:00]

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

[commit-bot: I haz the power, 2017-04-24 18:21:01]

Try jobs failed on following builders:
  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_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/24848)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/24763)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/39663)

[bbudge, 2017-04-24 18:24:49]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2017-04-24 18:24:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, georgia.kouveli@arm.com
Link to the patchset: https://codereview.chromium.org/2804883008/#ps160001
(title: "Rebase, reformat.")

[commit-bot: I haz the power, 2017-04-24 18:25:01]

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, 2017-04-24 18:53:17]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493058289476870,
"parent_rev": "6c0e81bd48a1db3183e0f94c04cb89708da2c61e", "commit_rev":
"a71c338d9e24e55b47125108a0fce754076751d0"}

[commit-bot: I haz the power, 2017-04-24 18:53:23]

Description was changed from

==========
[WASM SIMD] Implement horizontal add for float and integer types.
- Adds new F32x4AddHoriz, I32x4AddHoriz, etc. to WASM opcodes.
- Implements them for ARM.

LOG=N
BUG=v8:6020
==========

to

==========
[WASM SIMD] Implement horizontal add for float and integer types.
- Adds new F32x4AddHoriz, I32x4AddHoriz, etc. to WASM opcodes.
- Implements them for ARM.

LOG=N
BUG=v8:6020

Review-Url: https://codereview.chromium.org/2804883008
Cr-Commit-Position: refs/heads/master@{#44812}
Committed:
https://chromium.googlesource.com/v8/v8/+/a71c338d9e24e55b47125108a0fce754076...
==========

[commit-bot: I haz the power, 2017-04-24 18:53:25]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/a71c338d9e24e55b47125108a0fce754076...