[Turbofan] Add ARM support for simd128 moves and swaps.

[Turbofan] Add ARM support for simd128 moves and swaps.
- Adds vmov, vswp instructions for QwNeonRegisters.
- Refactors existing vswp implementation, moves non-Neon adaption to
MacroAssembler.
- Adds simd128 support to CodeGenerator AssembleMove, AssembleSwap.

LOG=N
BUG=v8:4124
Committed: https://crrev.com/ef3f125d373d971c30efe53756507512e316a78a
Cr-Commit-Position: refs/heads/master@{#41291}

[bbudge, 2016-11-23 04:04:13]

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

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

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-23 04:32:26]

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

[commit-bot: I haz the power, 2016-11-23 04:32:27]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-11-23 05:10:46]

bbudge@chromium.org changed reviewers:
+ bmeurer@chromium.org

[bbudge, 2016-11-23 05:10:46]

Please have a look or route appropriately.

[Benedikt Meurer, 2016-11-23 05:18:32]

bmeurer@chromium.org changed reviewers:
+ rodolph.perfetta@gmail.com

[Benedikt Meurer, 2016-11-23 05:18:32]

Rodolph, please have someone from your team to look into this.

[Rodolph Perfetta, 2016-11-23 11:24:41]

On 2016/11/23 05:18:32, Benedikt Meurer wrote:
> Rodolph, please have someone from your team to look into this.

We're looking at it.

[martyn.capewell, 2016-11-23 20:08:45]

martyn.capewell@arm.com changed reviewers:
+ martyn.capewell@arm.com

[martyn.capewell, 2016-11-23 20:08:47]

https://codereview.chromium.org/2523933002/diff/1/test/cctest/test-assembler-...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/2523933002/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-arm.cc:1293: __ vmov(q0, q1);
The results from these vmov instructions aren't written anywhere, so how do they
get tested.

https://codereview.chromium.org/2523933002/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-arm.cc:2917: __ vswp(q8, q9);
What does this test?

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

https://codereview.chromium.org/2523933002/diff/20001/src/arm/assembler-arm.h...
src/arm/assembler-arm.h:1333: void vswp(DwVfpRegister dst, DwVfpRegister src);
I guess this is a deliberate change, but I named the arguments srcdst here to
make it clear that we read and write both arguments.

https://codereview.chromium.org/2523933002/diff/20001/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

https://codereview.chromium.org/2523933002/diff/20001/src/arm/constants-arm.h...
src/arm/constants-arm.h:675: if (pre == kDoublePrecision) {
As Q is encoded in the same way as D, but as Q*2, how about something like:
if (pre == kSinglePrecision) {
  return (Bits(four_bit + 3, four_bit) << 1) | Bit(one_bit);
} else {
  int reg_num = (Bit(one_bit) << 4) | Bits(four_bit + 3, four_bit);
  if (pre == kDoublePrecision) {
    return reg_num;
  }
  DCHECK_EQ(kSimd128Precision, pre);
  DCHECK_EQ(reg_num & 1, 0);
  return reg_num / 2;
}

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

https://codereview.chromium.org/2523933002/diff/20001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1807: // vmov Qd, Qm
There's no disassembler test for this.

https://codereview.chromium.org/2523933002/diff/20001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1851: SNPrintF(out_buffer_ + out_buffer_pos_, "vswp q%d,
q%d", Vd, Vm);
There's no disassembler test for this.

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1914: __ vstr(src.low(), dst);
TODO here to implement this using vst1, when the instruction is available.

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1934: __ vldr(dst.high(), src);
TODO here to implement this using vld1, when the instruction is available.

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1951: __ vldr(kScratchDoubleReg, src);
TODO here to implement this using vld1/vst1 and restore zero register, when the
instructions are available.

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:2040: __ vmov(kDoubleRegZero, 0);  //
Restore the 0 register.
This is relatively expensive, which is why we statically allocated a zero
register in the first place. Is it possible to defer restoring the zero register
until after all swaps are completed?

Alternatively, you could implement the NEON veor instruction, and clear it by
exclusive orr with itself.

[bbudge, 2016-11-24 02:21:20]

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

[commit-bot: I haz the power, 2016-11-24 02:21:31]

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

[bbudge, 2016-11-24 02:22:53]

Thanks for the quick review!

I think I've addressed everything.

https://codereview.chromium.org/2523933002/diff/1/test/cctest/test-assembler-...
File test/cctest/test-assembler-arm.cc (right):

https://codereview.chromium.org/2523933002/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-arm.cc:1293: __ vmov(q0, q1);
On 2016/11/23 20:08:46, martyn.capewell wrote:
> The results from these vmov instructions aren't written anywhere, so how do
they
> get tested.

Done.

https://codereview.chromium.org/2523933002/diff/1/test/cctest/test-assembler-...
test/cctest/test-assembler-arm.cc:2917: __ vswp(q8, q9);
On 2016/11/23 20:08:46, martyn.capewell wrote:
> What does this test?

I eliminated this. Kept the test for lower q-regs.

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

https://codereview.chromium.org/2523933002/diff/20001/src/arm/assembler-arm.h...
src/arm/assembler-arm.h:1333: void vswp(DwVfpRegister dst, DwVfpRegister src);
On 2016/11/23 20:08:46, martyn.capewell wrote:
> I guess this is a deliberate change, but I named the arguments srcdst here to
> make it clear that we read and write both arguments.

I'll change it back.

https://codereview.chromium.org/2523933002/diff/20001/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

https://codereview.chromium.org/2523933002/diff/20001/src/arm/constants-arm.h...
src/arm/constants-arm.h:675: if (pre == kDoublePrecision) {
On 2016/11/23 20:08:46, martyn.capewell wrote:
> As Q is encoded in the same way as D, but as Q*2, how about something like:
> if (pre == kSinglePrecision) {
>   return (Bits(four_bit + 3, four_bit) << 1) | Bit(one_bit);
> } else {
>   int reg_num = (Bit(one_bit) << 4) | Bits(four_bit + 3, four_bit);
>   if (pre == kDoublePrecision) {
>     return reg_num;
>   }
>   DCHECK_EQ(kSimd128Precision, pre);
>   DCHECK_EQ(reg_num & 1, 0);
>   return reg_num / 2;
> }

Done.

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

https://codereview.chromium.org/2523933002/diff/20001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1807: // vmov Qd, Qm
On 2016/11/23 20:08:46, martyn.capewell wrote:
> There's no disassembler test for this.

I didn't realize there were disassembler tests. Added tests for vmov, vswp, and
veor.

https://codereview.chromium.org/2523933002/diff/20001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1809: int Vm = (instr->Bit(7) << 3) | instr->VmValue() >>
1;
I also converted this to use the VfpXRegister functions.

https://codereview.chromium.org/2523933002/diff/20001/src/arm/disasm-arm.cc#n...
src/arm/disasm-arm.cc:1851: SNPrintF(out_buffer_ + out_buffer_pos_, "vswp q%d,
q%d", Vd, Vm);
On 2016/11/23 20:08:46, martyn.capewell wrote:
> There's no disassembler test for this.

Done.

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1914: __ vstr(src.low(), dst);
On 2016/11/23 20:08:46, martyn.capewell wrote:
> TODO here to implement this using vst1, when the instruction is available.

I switched to vst1.

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1934: __ vldr(dst.high(), src);
On 2016/11/23 20:08:46, martyn.capewell wrote:
> TODO here to implement this using vld1, when the instruction is available.

Switched to vld1.

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1951: __ vldr(kScratchDoubleReg, src);
On 2016/11/23 20:08:46, martyn.capewell wrote:
> TODO here to implement this using vld1/vst1 and restore zero register, when
the
> instructions are available.

Switched to vld1/vst1.

https://codereview.chromium.org/2523933002/diff/20001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:2040: __ vmov(kDoubleRegZero, 0);  //
Restore the 0 register.
On 2016/11/23 20:08:46, martyn.capewell wrote:
> This is relatively expensive, which is why we statically allocated a zero
> register in the first place. Is it possible to defer restoring the zero
register
> until after all swaps are completed?
> 
> Alternatively, you could implement the NEON veor instruction, and clear it by
> exclusive orr with itself.

Added veor for d- and q- registers.

[commit-bot: I haz the power, 2016-11-24 02:50:20]

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

[commit-bot: I haz the power, 2016-11-24 02:50:20]

Dry run: This issue passed the CQ dry run.

[martyn.capewell, 2016-11-24 11:40:58]

Looking good. Only a couple of minor issues remaining.

https://codereview.chromium.org/2523933002/diff/40001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2523933002/diff/40001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1917: // // TODO(bbudge) Use vst1 when
it's available.
I hadn't realised vld1/vst1 had already been implemented. I guess these comments
can go now.

As these are NEON instructions, this will make NEON a requirement for
Simd128Register support. Is that the expectation?

I notice that in other parts of this patch, there is non-NEON fallback code for
Q-sized operations, eg. in the macro assembler for Move and Swap Q. If you can
assume Simd128 implies Q registers, then Q registers implies NEON, and the
fallback code for Q in the macro assembler can be removed.

Otherwise, you may need to make a macro Vld1 which selects between vld1 and a
vldr sequence on D registers, depending on NEON availability, and similarly for
Vst1.

https://codereview.chromium.org/2523933002/diff/40001/test/cctest/test-disasm...
File test/cctest/test-disasm-arm.cc (right):

https://codereview.chromium.org/2523933002/diff/40001/test/cctest/test-disasm...
test/cctest/test-disasm-arm.cc:951: COMPARE(veor(q0, q1, q2),
Add a test for veor on D registers.

[bbudge, 2016-11-24 13:47:59]

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

[bbudge, 2016-11-24 13:48:05]

https://codereview.chromium.org/2523933002/diff/40001/src/compiler/arm/code-g...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2523933002/diff/40001/src/compiler/arm/code-g...
src/compiler/arm/code-generator-arm.cc:1917: // // TODO(bbudge) Use vst1 when
it's available.
On 2016/11/24 11:40:58, martyn.capewell wrote:
> I hadn't realised vld1/vst1 had already been implemented. I guess these
comments
> can go now.
> 
> As these are NEON instructions, this will make NEON a requirement for
> Simd128Register support. Is that the expectation?
> 
> I notice that in other parts of this patch, there is non-NEON fallback code
for
> Q-sized operations, eg. in the macro assembler for Move and Swap Q. If you can
> assume Simd128 implies Q registers, then Q registers implies NEON, and the
> fallback code for Q in the macro assembler can be removed.
> 
> Otherwise, you may need to make a macro Vld1 which selects between vld1 and a
> vldr sequence on D registers, depending on NEON availability, and similarly
for
> Vst1.

I forgot to remove these commented out sections.

It's true that we should not be emitting q-register moves/swaps here if NEON
isn't supported (in which case we do SIMD scalar lowering) so I think we should
remove the fallback code for q-register move/swap in the macro assembler.

https://codereview.chromium.org/2523933002/diff/40001/test/cctest/test-disasm...
File test/cctest/test-disasm-arm.cc (right):

https://codereview.chromium.org/2523933002/diff/40001/test/cctest/test-disasm...
test/cctest/test-disasm-arm.cc:951: COMPARE(veor(q0, q1, q2),
On 2016/11/24 11:40:58, martyn.capewell wrote:
> Add a test for veor on D registers.

Done.

[commit-bot: I haz the power, 2016-11-24 13:48: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-24 14:27:10]

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

[commit-bot: I haz the power, 2016-11-24 14:27:11]

Dry run: This issue passed the CQ dry run.

[martyn.capewell, 2016-11-24 17:13:24]

lgtm

[bbudge, 2016-11-24 17:22:07]

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

[commit-bot: I haz the power, 2016-11-24 17:22: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-11-24 17:50:29]

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

[commit-bot: I haz the power, 2016-11-24 17:50:30]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-11-24 18:42:12]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2016-11-24 18:42:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
martyn.capewell@arm.com
Link to the patchset: https://codereview.chromium.org/2523933002/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-24 18:42:23]

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

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

[commit-bot: I haz the power, 2016-11-24 18:45:14]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29364)

[bbudge, 2016-11-25 15:27:18]

On 2016/11/24 18:45:14, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_presubmit on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29364)

Needs an OWNERS review for src/compiler.

[Benedikt Meurer, 2016-11-25 18:38:12]

LGTM, thanks.

[bbudge, 2016-11-25 18:44:01]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2016-11-25 18:44:06]

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

[bbudge, 2016-11-25 18:53:46]

The CQ bit was unchecked by bbudge@chromium.org

[bbudge, 2016-11-25 19:01:45]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2016-11-25 19:01: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-11-25 19:36:46]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480100505898960,
"parent_rev": "e4d20be4e4e25d4d47f08ef445a933262f3af7d3", "commit_rev":
"b42d8f6a9d73f8733c7ea598b87d8c4cbdcfa338"}

[commit-bot: I haz the power, 2016-11-25 19:36:53]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-25 19:37:15]

Description was changed from

==========
[Turbofan] Add ARM support for simd128 moves and swaps.
- Adds vmov, vswp instructions for QwNeonRegisters.
- Refactors existing vswp implementation, moves non-Neon adaption to
MacroAssembler.
- Adds simd128 support to CodeGenerator AssembleMove, AssembleSwap.


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

to

==========
[Turbofan] Add ARM support for simd128 moves and swaps.
- Adds vmov, vswp instructions for QwNeonRegisters.
- Refactors existing vswp implementation, moves non-Neon adaption to
MacroAssembler.
- Adds simd128 support to CodeGenerator AssembleMove, AssembleSwap.

LOG=N
BUG=v8:4124

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

[commit-bot: I haz the power, 2016-11-25 19:37:16]

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

[Michael Achenbach, 2016-11-29 11:13:32]

FYI: This causes check failures on chromebooks:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm/builds/1629
https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm%20-%20debug/bu...