[wasm] Implementation of Word32PairShr and Word32PairSar on arm.

[wasm] Implementation of Word32PairShr and Word32PairSar on arm.

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

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

Committed: https://crrev.com/049341f054f1568d5b93f5b55e11c68ae0ffee00
Cr-Commit-Position: refs/heads/master@{#34769}

[Rodolph Perfetta (ARM), 2016-03-10 14:48:09]

rodolph.perfetta@arm.com changed reviewers:
+ rodolph.perfetta@arm.com

[Rodolph Perfetta (ARM), 2016-03-10 14:48:10]

looks good, only a few minor comments

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

https://codereview.chromium.org/1778893004/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1156: cmp_raw_immediate(shift, 32);
If you move the rsb up here you can use it to do the compare:

  rsb(scratch, shift, Operand(32), SetCC);
  b(gt, &less_than_32);
  ...

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

https://codereview.chromium.org/1778893004/diff/1/src/compiler/arm/instructio...
src/compiler/arm/instruction-selector-arm.cc:812: // We use g.UseUniqueRegister
here for InputAt(0) and InputAt(2) to to
Comment doesn't match the code, UseUniqueRegister is used for Input(1) and
imput(2) below.

https://codereview.chromium.org/1778893004/diff/1/src/compiler/arm/instructio...
src/compiler/arm/instruction-selector-arm.cc:835: // We use g.UseUniqueRegister
here for InputAt(0) and InputAt(2) to to
ditto

[ahaas, 2016-03-10 15:33:35]

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

https://codereview.chromium.org/1778893004/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1156: cmp_raw_immediate(shift, 32);
On 2016/03/10 at 14:48:10, Rodolph Perfetta (ARM) wrote:
> If you move the rsb up here you can use it to do the compare:
> 
>   rsb(scratch, shift, Operand(32), SetCC);
>   b(gt, &less_than_32);
>   ...

Thanks, I do the same now for LslPair. If I use SetCC, should I also add the
"DCHECK_EQ(LeaveCC, i.OutputSBit());"? Could you explain me what this DCHECK
actually checks?

[ahaas, 2016-03-10 18:00:53]

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

https://codereview.chromium.org/1778893004/diff/1/src/arm/macro-assembler-arm...
src/arm/macro-assembler-arm.cc:1156: cmp_raw_immediate(shift, 32);
On 2016/03/10 at 14:48:10, Rodolph Perfetta (ARM) wrote:
> If you move the rsb up here you can use it to do the compare:
> 
>   rsb(scratch, shift, Operand(32), SetCC);
>   b(gt, &less_than_32);
>   ...

Done.

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

https://codereview.chromium.org/1778893004/diff/1/src/compiler/arm/instructio...
src/compiler/arm/instruction-selector-arm.cc:812: // We use g.UseUniqueRegister
here for InputAt(0) and InputAt(2) to to
On 2016/03/10 at 14:48:10, Rodolph Perfetta (ARM) wrote:
> Comment doesn't match the code, UseUniqueRegister is used for Input(1) and
imput(2) below.

Done.

https://codereview.chromium.org/1778893004/diff/1/src/compiler/arm/instructio...
src/compiler/arm/instruction-selector-arm.cc:835: // We use g.UseUniqueRegister
here for InputAt(0) and InputAt(2) to to
On 2016/03/10 at 14:48:10, Rodolph Perfetta (ARM) wrote:
> ditto

Done.

[Rodolph Perfetta, 2016-03-11 14:58:04]

On 2016/03/10 15:33:35, ahaas wrote:
>
https://codereview.chromium.org/1778893004/diff/1/src/arm/macro-assembler-arm.cc
> File src/arm/macro-assembler-arm.cc (right):
> 
>
https://codereview.chromium.org/1778893004/diff/1/src/arm/macro-assembler-arm...
> src/arm/macro-assembler-arm.cc:1156: cmp_raw_immediate(shift, 32);
> On 2016/03/10 at 14:48:10, Rodolph Perfetta (ARM) wrote:
> > If you move the rsb up here you can use it to do the compare:
> > 
> >   rsb(scratch, shift, Operand(32), SetCC);
> >   b(gt, &less_than_32);
> >   ...
> 
> Thanks, I do the same now for LslPair. If I use SetCC, should I also add the
> "DCHECK_EQ(LeaveCC, i.OutputSBit());"? Could you explain me what this DCHECK
> actually checks?

OutputSbit() is SetCC if the ArmOpcode is expected to set the flags for the
following branch or condition materialisation. Some instructions such as kArmDiv
cannot set the flags so if OutputSBit is SetCC we have an error, hence the
DCHECK. Note that confusingly if OutputSBit is LeaveCC it doesn't mean the
opcode implementation cannot generate an instruction which set the flags, just
that the following instruction does not plan to use the flags.

Word32Sar can set the flags so the code if (x >> n) { ... } could be compiled to
something on the lines of: Word32Sar x, n, SetCC; Branch. Since Word32PairSar
cannot set the flags the code generated would be more like Word32PairAsr x, n;
Tst previous output; Branch. Adding DCHECK_EQ(LeaveCC, i.OutputSBit()) here
helps find logical faults in the architecture specific backend.

[Rodolph Perfetta (ARM), 2016-03-11 15:00:42]

LGTM, still needs an owner approval.

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

https://codereview.chromium.org/1778893004/diff/20001/src/arm/macro-assembler...
src/arm/macro-assembler-arm.cc:1168: 
nit: did you intended the blank line?

[titzer, 2016-03-14 09:27:34]

On 2016/03/11 15:00:42, Rodolph Perfetta (ARM) wrote:
> LGTM, still needs an owner approval.
> 
>
https://codereview.chromium.org/1778893004/diff/20001/src/arm/macro-assembler...
> File src/arm/macro-assembler-arm.cc (right):
> 
>
https://codereview.chromium.org/1778893004/diff/20001/src/arm/macro-assembler...
> src/arm/macro-assembler-arm.cc:1168: 
> nit: did you intended the blank line?

lgtm

[ahaas, 2016-03-15 06:58:22]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-03-15 06:58:23]

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

[commit-bot: I haz the power, 2016-03-15 06:58:25]

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

[commit-bot: I haz the power, 2016-03-15 07:15:55]

Committed patchset #5 (id:80001)

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

Description was changed from

==========
[wasm] Implementation of Word32PairShr and Word32PairSar on arm.

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

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

to

==========
[wasm] Implementation of Word32PairShr and Word32PairSar on arm.

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

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

Committed: https://crrev.com/049341f054f1568d5b93f5b55e11c68ae0ffee00
Cr-Commit-Position: refs/heads/master@{#34769}
==========

[commit-bot: I haz the power, 2016-03-15 07:17:48]

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