Subzero. ARM32. Vector shifts.

Subzero. ARM32. Vector shifts.

BUG=
R=kschimpf@google.com, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=15e77d4698df65dfce5072b59df18500ff56daa3

[John, 2016-04-13 14:38:20]

jpp@chromium.org changed reviewers:
+ eholk@chromium.org, kschimpf@google.com, seh@chromium.org,
stichnot@chromium.org

[Jim Stichnoth, 2016-04-13 15:53:24]

half-science-dog lgtm

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.def#n...
src/IceInstARM32.def:90: //#define X(tag, elementty, int_width, fp_width, sbits,
ubits, rraddr, shaddr)
Update comment to include new macro args.

[Karl, 2016-04-13 16:17:37]

Otherwise: LGTM.

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:223: switch (SignType) {
This switch appears in multiple places. Consider factoring out into a helper
function?

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:804: Asm->vnegqs(ElmtTy, Dest, getSrc(0));
Is it necessary to factor out ElmtTy here and pass into vnegqs? Why not have
vnegqs do this?

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:846: Asm->vshlqu(ElmtTy, Dest, getSrc(0), getSrc(1));
Similar question here. Should we be forcing the passing of ElmtTy to vshlqu and
vshlqi when they get get it from Dest?

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/asr.ll (right):

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/asr.ll:76: ; DIS:      20:          f3b923c2
No need to check the bytes generated for IASM. the 4th RUN command will check
this using DIS.

Simply check that it wasn't a text fix up. That is:

; IASM-NOT: vneg.s32  q1, q1
; IASM-NOT: vshl.s32 q0, q0, a1

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/asr.ll:101: ; IASM:         .byte 0xc2
Same here.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/asr.ll:124: ; IASM:         .byte 0xc2
Same here.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/lsl.ll (right):

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsl.ll:78: ; IASM:         .byte 0x40
Same here.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsl.ll:95: ; IASM:         .byte 0x40
Same here.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsl.ll:112: ; IASM:         .byte 0x40
Same here.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/lsr.ll (right):

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsr.ll:80: ; IASM:         .byte 0xc2
Same here (and below).

[John, 2016-04-13 19:57:18]

Description was changed from

==========
Subzero. ARM32. Vector shifts.

BUG=
==========

to

==========
Subzero. ARM32. Vector shifts.

BUG=
R=kschimpf@google.com, stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[John, 2016-04-13 19:57:19]

Committed patchset #8 (id:140001) manually as
15e77d4698df65dfce5072b59df18500ff56daa3 (presubmit successful).

[John, 2016-04-15 13:20:57]

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:223: switch (SignType) {
On 2016/04/13 16:17:36, Karl wrote:
> This switch appears in multiple places. Consider factoring out into a helper
> function?

Done.

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:804: Asm->vnegqs(ElmtTy, Dest, getSrc(0));
On 2016/04/13 16:17:36, Karl wrote:
> Is it necessary to factor out ElmtTy here and pass into vnegqs? Why not have
> vnegqs do this?

No, it is not, but there is precedent in this file (i.e., vaddqi, vmulqi, and
vsubqi), so I went for consistency. Maybe this should be cleaned (follow up cl)?
Let me know, and I will make this change. :)

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.cpp#n...
src/IceInstARM32.cpp:846: Asm->vshlqu(ElmtTy, Dest, getSrc(0), getSrc(1));
On 2016/04/13 16:17:36, Karl wrote:
> Similar question here. Should we be forcing the passing of ElmtTy to vshlqu
and
> vshlqi when they get get it from Dest?

Acknowledged.

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.def
File src/IceInstARM32.def (right):

https://codereview.chromium.org/1881623002/diff/100001/src/IceInstARM32.def#n...
src/IceInstARM32.def:90: //#define X(tag, elementty, int_width, fp_width, sbits,
ubits, rraddr, shaddr)
On 2016/04/13 15:53:24, stichnot wrote:
> Update comment to include new macro args.

Done.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/asr.ll (right):

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/asr.ll:76: ; DIS:      20:          f3b923c2
On 2016/04/13 16:17:36, Karl wrote:
> No need to check the bytes generated for IASM. the 4th RUN command will check
> this using DIS.
> 
> Simply check that it wasn't a text fix up. That is:
> 
> ; IASM-NOT: vneg.s32  q1, q1
> ; IASM-NOT: vshl.s32 q0, q0, a1

Done.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/asr.ll:101: ; IASM:         .byte 0xc2
On 2016/04/13 16:17:36, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/asr.ll:124: ; IASM:         .byte 0xc2
On 2016/04/13 16:17:36, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/lsl.ll (right):

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsl.ll:78: ; IASM:         .byte 0x40
On 2016/04/13 16:17:37, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsl.ll:95: ; IASM:         .byte 0x40
On 2016/04/13 16:17:37, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsl.ll:112: ; IASM:         .byte 0x40
On 2016/04/13 16:17:37, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
File tests_lit/assembler/arm32/lsr.ll (right):

https://codereview.chromium.org/1881623002/diff/100001/tests_lit/assembler/ar...
tests_lit/assembler/arm32/lsr.ll:80: ; IASM:         .byte 0xc2
On 2016/04/13 16:17:37, Karl wrote:
> Same here (and below).

Done.

[Karl, 2016-04-15 14:27:49]

lgtm