[ARM] Add NEON instructions for implementing SIMD.

[ARM] Add NEON instructions for implementing SIMD.
- Adds vabs, vneg, vmul, vext, vzip, vrev instructions.
- Adds Swizzle function to macro assembler.
- Simplifies if-else logic in disassembler, simulator, for Neon special.
- Some refactoring of Neon assembler, macro-assembler tests.

LOG=N
BUG=v8:4124
Review-Url: https://codereview.chromium.org/2579913002
Cr-Commit-Position: refs/heads/master@{#41781}
Committed: https://chromium.googlesource.com/v8/v8/+/431223f34fafec07b7c42f6fa2fb832b82d86c94

[bbudge, 2016-12-15 23:53:38]

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

[commit-bot: I haz the power, 2016-12-15 23:53:45]

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-12-15 23:56:55]

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

[commit-bot: I haz the power, 2016-12-15 23:56:56]

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/17906)

[bbudge, 2016-12-16 00:02:16]

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

[commit-bot: I haz the power, 2016-12-16 00:02:21]

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-12-16 00:30:10]

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

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

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-12-16 01:14:56]

bbudge@chromium.org changed reviewers:
+ rodolph.perfetta@arm.com

[bbudge, 2016-12-16 01:14:57]

Another batch of NEON instructions. Fun!

[Rodolph Perfetta (ARM), 2016-12-16 20:50:12]

https://codereview.chromium.org/2579913002/diff/40001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:4081: int vd, d;
DCHECK is_float => size is Neon32

https://codereview.chromium.org/2579913002/diff/40001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:4317: emit(0x1E7U * B23 | d * B22 | 0x3 * B20 | sz *
B18 | 2 * B16 | vd * B12 | B8 |
no need for B8, you have 0x3 * B7 to cover that bit.

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

https://codereview.chromium.org/2579913002/diff/40001/src/arm/assembler-arm.h...
src/arm/assembler-arm.h:1392: void vrev(NeonSize op_size, NeonSize element_size,
const QwNeonRegister dst,
would you consider having 3 instructions here:
 * vrev16
 * vrev32
 * vrev64
This would match the ARM documentation and mirror the integer instructions
(rev16, etc). This would also make the parameters passed more in line with other
neon instructions (first parameter is the element size).

What do you think?

https://codereview.chromium.org/2579913002/diff/40001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:2080: } else if (instr->Opc1Value() == 0x7 &&
instr->Bits(11, 7) == 0x18 &&
this case has already been handled line 1998

https://codereview.chromium.org/2579913002/diff/40001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:2087: } else if (instr->Opc1Value() == 0x7 &&
instr->Bits(11, 10) == 0x2 &&
already handled line 2031

https://codereview.chromium.org/2579913002/diff/40001/src/arm/macro-assembler...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:1199: case 0x1032:  // Swap top and bottom.
vext(dst, src, src, 8);

https://codereview.chromium.org/2579913002/diff/40001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:1214: if (lane_code >=
SwVfpRegister::kMaxNumRegisters) {
TODO: use vdup (vdup.32 dst, Dsrc[lane]) once implemented.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4307: for (int i = 0; i < 4; i += 2) {
you refactored other Neon16 instructions to use uint16_t and memcpy in/out.
Could you do the same here?

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4433: if (s[i] < 0) s[i] = -s[i];
any reasons you are using std::abs above and the negation here?

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4440: int32_t* as_signed =
reinterpret_cast<int32_t*>(&src[i]);
reinterpret_cast the pointer to the array, the code will be nicer to read.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4476: int32_t* as_signed =
reinterpret_cast<int32_t*>(&src[i]);
as for vabs, why not reinterpreting the pointer to the array.

https://codereview.chromium.org/2579913002/diff/40001/test/cctest/test-assemb...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-arm.cc:1416: __ vmov(s1, -0);
-0.0 otherwise this will move 0 into s1.

https://codereview.chromium.org/2579913002/diff/40001/test/cctest/test-macro-...
File test/cctest/test-macro-assembler-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/test/cctest/test-macro-...
test/cctest/test-macro-assembler-arm.cc:469: __ bx(lr);
nit: unnecessary.

[bbudge, 2016-12-17 01:07:43]

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

[commit-bot: I haz the power, 2016-12-17 01:07:47]

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

[bbudge, 2016-12-17 01:08:32]

Thanks Rodolph. I think I got everything. PTAL

https://codereview.chromium.org/2579913002/diff/40001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:4081: int vd, d;
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> DCHECK is_float => size is Neon32

Done.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:4317: emit(0x1E7U * B23 | d * B22 | 0x3 * B20 | sz *
B18 | 2 * B16 | vd * B12 | B8 |
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> no need for B8, you have 0x3 * B7 to cover that bit.

Good catch, done!

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

https://codereview.chromium.org/2579913002/diff/40001/src/arm/assembler-arm.h...
src/arm/assembler-arm.h:1392: void vrev(NeonSize op_size, NeonSize element_size,
const QwNeonRegister dst,
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> would you consider having 3 instructions here:
>  * vrev16
>  * vrev32
>  * vrev64
> This would match the ARM documentation and mirror the integer instructions
> (rev16, etc). This would also make the parameters passed more in line with
other
> neon instructions (first parameter is the element size).
> 
> What do you think?

I think it's a good idea. Done.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:2080: } else if (instr->Opc1Value() == 0x7 &&
instr->Bits(11, 7) == 0x18 &&
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> this case has already been handled line 1998

Another nice catch, I had a bad merge somewhere. Done.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:2087: } else if (instr->Opc1Value() == 0x7 &&
instr->Bits(11, 10) == 0x2 &&
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> already handled line 2031

Done.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/macro-assembler...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:1199: case 0x1032:  // Swap top and bottom.
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> vext(dst, src, src, 8);

Nice, done.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:1214: if (lane_code >=
SwVfpRegister::kMaxNumRegisters) {
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> TODO: use vdup (vdup.32 dst, Dsrc[lane]) once implemented.

Done.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4307: for (int i = 0; i < 4; i += 2) {
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> you refactored other Neon16 instructions to use uint16_t and memcpy in/out.
> Could you do the same here?

Yep. Done.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4433: if (s[i] < 0) s[i] = -s[i];
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> any reasons you are using std::abs above and the negation here?

Typo. Wonder why the test passes. Fixed.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4440: int32_t* as_signed =
reinterpret_cast<int32_t*>(&src[i]);
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> reinterpret_cast the pointer to the array, the code will be nicer to read.

Done.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4441: if (*as_signed < 0) *as_signed = -*as_signed;
Also, changed this to std::abs.

https://codereview.chromium.org/2579913002/diff/40001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4476: int32_t* as_signed =
reinterpret_cast<int32_t*>(&src[i]);
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> as for vabs, why not reinterpreting the pointer to the array.

Done.

https://codereview.chromium.org/2579913002/diff/40001/test/cctest/test-assemb...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-arm.cc:1416: __ vmov(s1, -0);
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> -0.0 otherwise this will move 0 into s1.

Done.

https://codereview.chromium.org/2579913002/diff/40001/test/cctest/test-macro-...
File test/cctest/test-macro-assembler-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/40001/test/cctest/test-macro-...
test/cctest/test-macro-assembler-arm.cc:469: __ bx(lr);
On 2016/12/16 20:50:11, Rodolph Perfetta (ARM) wrote:
> nit: unnecessary.

Done.

[commit-bot: I haz the power, 2016-12-17 01:35:17]

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

[commit-bot: I haz the power, 2016-12-17 01:35:17]

Dry run: This issue passed the CQ dry run.

[Rodolph Perfetta (ARM), 2016-12-17 13:44:32]

lgtm

https://codereview.chromium.org/2579913002/diff/80001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/80001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4428: if (s[i] < 0) s[i] = std::abs(s[i]);
very minor nit: sometime you call abs when negative (Neon8 and Neon16) sometime
you call abs unconditionally. it wodl be better to have a unified pattern.

[bbudge, 2016-12-17 19:36:39]

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

[commit-bot: I haz the power, 2016-12-17 19:36: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-12-17 20:02:46]

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

[commit-bot: I haz the power, 2016-12-17 20:02:46]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-12-17 21:54:29]

Description was changed from

==========
[ARM] Add NEON instructions for implementing SIMD.
- Adds vabs, vneg, vmul, vext, vzip, vrev instructions.
- Adds Swizzle function to macro assembler.
- Simplifies if-else logic in disassembler, simulator, for Neon special.
- Some refactoring of Neon assembler, macro-assembler tests.

LOG=N
BUG=v8:4124
==========

to

==========
[ARM] Add NEON instructions for implementing SIMD.
- Adds vabs, vneg, vmul, vext, vzip, vrev instructions.
- Adds Swizzle function to macro assembler.
- Simplifies if-else logic in disassembler, simulator, for Neon special.
- Some refactoring of Neon assembler, macro-assembler tests.

LOG=N
BUG=v8:4124
==========

[bbudge, 2016-12-17 21:54:36]

https://codereview.chromium.org/2579913002/diff/80001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/2579913002/diff/80001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4428: if (s[i] < 0) s[i] = std::abs(s[i]);
On 2016/12/17 13:44:32, Rodolph Perfetta (ARM) wrote:
> very minor nit: sometime you call abs when negative (Neon8 and Neon16)
sometime
> you call abs unconditionally. it wodl be better to have a unified pattern.

Ugh. That's just wrong. Done here and below.

[bbudge, 2016-12-17 21:54:46]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2016-12-17 21:54:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
rodolph.perfetta@arm.com
Link to the patchset: https://codereview.chromium.org/2579913002/#ps100001
(title: "Review comments.")

[commit-bot: I haz the power, 2016-12-17 21:54:49]

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-12-17 21:56:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482011686189620,
"parent_rev": "01a216060061507c60b3cb167f61429a73fbb4b7", "commit_rev":
"431223f34fafec07b7c42f6fa2fb832b82d86c94"}

[commit-bot: I haz the power, 2016-12-17 21:56:33]

Description was changed from

==========
[ARM] Add NEON instructions for implementing SIMD.
- Adds vabs, vneg, vmul, vext, vzip, vrev instructions.
- Adds Swizzle function to macro assembler.
- Simplifies if-else logic in disassembler, simulator, for Neon special.
- Some refactoring of Neon assembler, macro-assembler tests.

LOG=N
BUG=v8:4124
==========

to

==========
[ARM] Add NEON instructions for implementing SIMD.
- Adds vabs, vneg, vmul, vext, vzip, vrev instructions.
- Adds Swizzle function to macro assembler.
- Simplifies if-else logic in disassembler, simulator, for Neon special.
- Some refactoring of Neon assembler, macro-assembler tests.

LOG=N
BUG=v8:4124

Review-Url: https://codereview.chromium.org/2579913002
Cr-Commit-Position: refs/heads/master@{#41781}
Committed:
https://chromium.googlesource.com/v8/v8/+/431223f34fafec07b7c42f6fa2fb832b82d...
==========

[commit-bot: I haz the power, 2016-12-17 21:56:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/431223f34fafec07b7c42f6fa2fb832b82d...