[turbofan] Implementation of Word32PairShl on arm.

[turbofan] Implementation of Word32PairShl on arm.

Implementation of https://codereview.chromium.org/1756863002 on arm.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com

Committed: https://crrev.com/5446dab6cbbc0d3330fa0faa9e7419a43d6058a2
Cr-Commit-Position: refs/heads/master@{#34557}

[ahaas, 2016-03-04 17:20:56]

https://codereview.chromium.org/1765973002/diff/1/src/compiler/arm/instructio...
File src/compiler/arm/instruction-selector-arm.cc (right):

https://codereview.chromium.org/1765973002/diff/1/src/compiler/arm/instructio...
src/compiler/arm/instruction-selector-arm.cc:783:
g.UseUniqueRegister(node->InputAt(1)),
InputAt(1) and NodeProperties::FindProjection(node, 1) could share a register.
Is it possible to say that in the code?

[martyn.capewell, 2016-03-04 18:54:56]

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

[martyn.capewell, 2016-03-04 18:54:58]

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

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1062: Label less_than_32;
Add some register alias assertions: perhaps !AreAliased() for all input
registers makes reasoning easier, but it may be helpful to allow dst_low ==
src_low and dst_high == src_high.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1066: eor(dst_low, dst_low, Operand(dst_low));
mov(dst_low, Operand(0))

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1066: eor(dst_low, dst_low, Operand(dst_low));
Switching zero and shift operations here would allow dst_low to alias src_low.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1075: lsl(dst_low, src_low, Operand(shift));
You can combine shift with orr here:
  lsl(dst_high, src_high, Operand(shift);
  orr(dst_high, dst_high, Operand(src_low, LSR, scratch));
  lsl(dst_low, src_low, Operand(shift));

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1081: Register scratch, uint32_t shift) {
scratch isn't used.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1082: Label less_than_32;
Add some register alias assertions. At the top level, I think (dst_low !=
dst_high) && (src_low != src_high) is enough.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1084: if (shift >= 32) {
Switch the zero and shift instructions here, so dst_low can alias src_low.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1085: eor(dst_low, dst_low, Operand(dst_low));
mov(dst_low, Operand(0))

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1088: } else if (shift == 0) {
Assert dst_low != src_high.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1089: mov(dst_low, src_low);
Use Move() here to eliminate redundant instructions when dst == src.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1095: lsl(dst_low, src_low, Operand(shift));
Combine shift with orr:
  lsl(dst_high, src_high, Operand(shift);
  orr(dst_high, dst_high, Operand(src_low, LSR, 32 - shift));
  lsl(dst_low, src_low, Operand(shift));

Also, assert (dst_high != src_low) && (src_high != dst_low).

[ahaas, 2016-03-07 13:40:49]

Thanks for the feedback. I adjusted the code accordingly. Register sharing is
now enforced for src_low and dst_low and allowed for src_high and dst_high.

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

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1062: Label less_than_32;
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> Add some register alias assertions: perhaps !AreAliased() for all input
registers makes reasoning easier, but it may be helpful to allow dst_low ==
src_low and dst_high == src_high.

I enforced src_low == dst_low now in the register allocator now, and I allow
src_high == dst_high.
I added !AreAliased() checks for (dst_low, src_high) and (dst_high, src_low).

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1066: eor(dst_low, dst_low, Operand(dst_low));
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> mov(dst_low, Operand(0))

Done.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1075: lsl(dst_low, src_low, Operand(shift));
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> You can combine shift with orr here:
>   lsl(dst_high, src_high, Operand(shift);
>   orr(dst_high, dst_high, Operand(src_low, LSR, scratch));
>   lsl(dst_low, src_low, Operand(shift));

Thanks, that's really helpful.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1081: Register scratch, uint32_t shift) {
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> scratch isn't used.

I removed it.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1082: Label less_than_32;
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> Add some register alias assertions. At the top level, I think (dst_low !=
dst_high) && (src_low != src_high) is enough.

Done.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1084: if (shift >= 32) {
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> Switch the zero and shift instructions here, so dst_low can alias src_low.

Done.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1085: eor(dst_low, dst_low, Operand(dst_low));
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> mov(dst_low, Operand(0))

Done.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1089: mov(dst_low, src_low);
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> Use Move() here to eliminate redundant instructions when dst == src.

Aha, that's useful. Thanks.

https://codereview.chromium.org/1765973002/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1095: lsl(dst_low, src_low, Operand(shift));
On 2016/03/04 at 18:54:57, martyn.capewell wrote:
> Combine shift with orr:
>   lsl(dst_high, src_high, Operand(shift);
>   orr(dst_high, dst_high, Operand(src_low, LSR, 32 - shift));
>   lsl(dst_low, src_low, Operand(shift));
> 
> Also, assert (dst_high != src_low) && (src_high != dst_low).

Done.

[martyn.capewell, 2016-03-07 17:46:47]

lgtm

[ahaas, 2016-03-07 17:56:49]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-03-07 17:56:49]

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/1765973002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-07 17:56:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765973002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765973002/60001

[commit-bot: I haz the power, 2016-03-07 18:08:43]

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

[commit-bot: I haz the power, 2016-03-07 18:08:44]

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

[titzer, 2016-03-07 18:16:04]

On 2016/03/07 18:08:44, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_presubmit on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11978)

lgtm

[ahaas, 2016-03-07 18:23:57]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2016-03-07 18:24:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765973002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765973002/60001

[commit-bot: I haz the power, 2016-03-07 18:46:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-07 18:47:18]

Description was changed from

==========
[turbofan] Implementation of Word32PairShl on arm.

Implementation of https://codereview.chromium.org/1756863002 on arm.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com
==========

to

==========
[turbofan] Implementation of Word32PairShl on arm.

Implementation of https://codereview.chromium.org/1756863002 on arm.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com

Committed: https://crrev.com/5446dab6cbbc0d3330fa0faa9e7419a43d6058a2
Cr-Commit-Position: refs/heads/master@{#34557}
==========

[commit-bot: I haz the power, 2016-03-07 18:47:19]

Patchset 4 (id:??) landed as
https://crrev.com/5446dab6cbbc0d3330fa0faa9e7419a43d6058a2
Cr-Commit-Position: refs/heads/master@{#34557}