[ARM] Add Neon shift instructions vshl, vshr.

[ARM] Add Neon shift instructions vshl, vshr.

LOG=N
BUG=v8:4124
Review-Url: https://codereview.chromium.org/2629223005
Cr-Commit-Position: refs/heads/master@{#42610}
Committed: https://chromium.googlesource.com/v8/v8/+/67244dcef1324fe790630cc51e8b4116c4916e15

[bbudge, 2017-01-16 21:48:26]

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

[commit-bot: I haz the power, 2017-01-16 21:48:33]

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-01-16 22:14:16]

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

[commit-bot: I haz the power, 2017-01-16 22:14:17]

Dry run: This issue passed the CQ dry run.

[bbudge, 2017-01-18 16:11:53]

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

[commit-bot: I haz the power, 2017-01-18 16:12:00]

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-01-18 16:38:45]

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

[commit-bot: I haz the power, 2017-01-18 16:38:46]

Dry run: This issue passed the CQ dry run.

[bbudge, 2017-01-18 17:19:13]

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

[martyn.capewell, 2017-01-19 14:04:27]

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

https://codereview.chromium.org/2629223005/diff/60001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:4433: int imm6 = size_in_bits + shift;
The encoding of imm6 differs between left and right shift.

For left, shift = imm6 - size => imm6 = size + shift.
For right, shift = 2*size - imm6 => imm6 = 2*size - shift.

This means the range of shifts differs too; left shift on 8-bit values allows
shifts of 0 to 7, right shift allows 1 to 8. It's worth asserting this.

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

https://codereview.chromium.org/2629223005/diff/60001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4247: src[i] >>= shift;
As vshr supports a right shift by the size of the element, this can hit a C++
implementation defined (or maybe undefined) behaviour.

[bbudge, 2017-01-19 21:57:59]

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

[commit-bot: I haz the power, 2017-01-19 21:58:06]

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

[bbudge, 2017-01-19 22:19:43]

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

https://codereview.chromium.org/2629223005/diff/60001/src/arm/assembler-arm.c...
src/arm/assembler-arm.cc:4433: int imm6 = size_in_bits + shift;
On 2017/01/19 14:04:27, martyn.capewell wrote:
> The encoding of imm6 differs between left and right shift.
> 
> For left, shift = imm6 - size => imm6 = size + shift.
> For right, shift = 2*size - imm6 => imm6 = 2*size - shift.
> 
> This means the range of shifts differs too; left shift on 8-bit values allows
> shifts of 0 to 7, right shift allows 1 to 8. It's worth asserting this.

Thanks for working it all out for me Martyn! Done.

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

https://codereview.chromium.org/2629223005/diff/60001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:4247: src[i] >>= shift;
On 2017/01/19 14:04:27, martyn.capewell wrote:
> As vshr supports a right shift by the size of the element, this can hit a C++
> implementation defined (or maybe undefined) behaviour.

I've added a helper method to simulate arithmetic right shift.

[commit-bot: I haz the power, 2017-01-19 22:32:40]

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

[commit-bot: I haz the power, 2017-01-19 22:32:41]

Dry run: This issue passed the CQ dry run.

[martyn.capewell, 2017-01-20 11:35:57]

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

https://codereview.chromium.org/2629223005/diff/80001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:576: return x >> shift | ~(~0U >> shift);
x is still potentially signed here, so shift >= sizeof(T) is implementation
defined. I think you need a std::make_unsigned.

[martyn.capewell, 2017-01-20 16:14:37]

Rodolph just pointed out the ArithmeticShiftRight() function in utils.h; it's
not templated and exploits potentially implementation defined behaviour. It
might be worth fixing and reusing it, rather than implementing another.

[bbudge, 2017-01-20 16:56:23]

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

https://codereview.chromium.org/2629223005/diff/80001/src/arm/simulator-arm.c...
src/arm/simulator-arm.cc:576: return x >> shift | ~(~0U >> shift);
On 2017/01/20 11:35:57, martyn.capewell wrote:
> x is still potentially signed here, so shift >= sizeof(T) is implementation
> defined. I think you need a std::make_unsigned.

Thanks, done.

[bbudge, 2017-01-20 16:56:34]

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

[commit-bot: I haz the power, 2017-01-20 16:56:36]

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-01-20 17:29:18]

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

[commit-bot: I haz the power, 2017-01-20 17:29:18]

Dry run: This issue passed the CQ dry run.

[bbudge, 2017-01-21 16:18:28]

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

[commit-bot: I haz the power, 2017-01-21 16:18:37]

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

[bbudge, 2017-01-21 16:21:14]

Switched to use fixed ArithmeticRightShift in utils.h

[commit-bot: I haz the power, 2017-01-21 16:40:59]

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

[commit-bot: I haz the power, 2017-01-21 16:41:00]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/16112)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21275)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[bbudge, 2017-01-22 16:16:14]

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

[commit-bot: I haz the power, 2017-01-22 16:16:24]

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-01-22 16:49:20]

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

[commit-bot: I haz the power, 2017-01-22 16:49:21]

Dry run: This issue passed the CQ dry run.

[martyn.capewell, 2017-01-22 17:16:53]

lgtm

[bbudge, 2017-01-23 17:46:20]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2017-01-23 17:46:26]

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-01-23 18:24:28]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485193580681420,
"parent_rev": "f9f5fc31fdea4ded8287d6bc8ee36e147b6412d4", "commit_rev":
"67244dcef1324fe790630cc51e8b4116c4916e15"}

[commit-bot: I haz the power, 2017-01-23 18:24:35]

Description was changed from

==========
[ARM] Add Neon shift instructions vshl, vshr.

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

to

==========
[ARM] Add Neon shift instructions vshl, vshr.

LOG=N
BUG=v8:4124

Review-Url: https://codereview.chromium.org/2629223005
Cr-Commit-Position: refs/heads/master@{#42610}
Committed:
https://chromium.googlesource.com/v8/v8/+/67244dcef1324fe790630cc51e8b4116c49...
==========

[commit-bot: I haz the power, 2017-01-23 18:24:36]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/67244dcef1324fe790630cc51e8b4116c49...