[Subzero][MIPS32] Implement icmp operation for i8, i16 operands

[Subzero][MIPS32] Implement icmp operation for i8, i16 operands

BUG=none
R=jpp@chromium.org

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

[sagar.thakur, 2016-05-31 13:45:38]

sagar.thakur@imgtec.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, kschimpf@google.com,
stichnot@chromium.org

[sagar.thakur, 2016-05-31 13:45:39]

Added lowering implementation of icmp with i8, i16 operands.
Added tests for icmp with i8, i16, i32 opeands.

[John, 2016-05-31 14:16:38]

https://codereview.chromium.org/2022063003/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2022063003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1179: _andi(Src0RT, Src0R, Mask);
For ARM, Subzero does not bother clearing the upper bits in non-32-bit
arithmetic, ensuring that writes to memory (and loads), and comparisons only
look at the "right" bits.

When comparing to N-bit integers (N is 1, 8, or 16), the ARM backend lsl the
operands (shifts left) and compares the resulting operands (no right shift.)

For example, if an operand is an i8, the lowering sequence would be:

mov T0, Src0
mov T1, Src1
lsl T0, T0, 24
cmp T0, T1 lsl 24

notice that by shifting the operand left by 24 bits, the operands most
significant bit (the sign bit) ends up in bit 32 of the destination register.

I think MIPS could do something similar (i.e., simply left shift the operands.)
What do you think? For the signed case this could save two instructions per
comparison.

[Jim Stichnoth, 2016-05-31 19:00:20]

L-G-T-M as long as John agrees.

https://codereview.chromium.org/2022063003/diff/1/tests_lit/llvm2ice_tests/ic...
File tests_lit/llvm2ice_tests/icmp.ll (right):

https://codereview.chromium.org/2022063003/diff/1/tests_lit/llvm2ice_tests/ic...
tests_lit/llvm2ice_tests/icmp.ll:4: ; RUN:   --filetype=asm --target mips32 -i
%s --args -O2 --skip-unimplemented \
Just a comment and an optional suggestion.

For an early-stage target lowering, and with filetype=asm, I like to lightly try
with both -Om1 and -O2.  Sometimes this reveals incorrect lowering and invalid
addressing modes (leading to llvm-mc errors).  Sometimes "make check-lit" just
works.  In other cases (including this one), pnacl-sz and llvm-mc generate valid
code, but FileCheck complains because the CHECK patterns don't match.

In this case, a cursory examination suggests that the failures are from
overly-specific register assumptions.  It might be worth making the patterns
more general, which could help future-proof the lit tests against future
register allocation or related changes.

[sagar.thakur, 2016-06-01 13:17:22]

https://codereview.chromium.org/2022063003/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2022063003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1179: _andi(Src0RT, Src0R, Mask);
On 2016/05/31 14:16:38, John wrote:
> For ARM, Subzero does not bother clearing the upper bits in non-32-bit
> arithmetic, ensuring that writes to memory (and loads), and comparisons only
> look at the "right" bits.
> 
> When comparing to N-bit integers (N is 1, 8, or 16), the ARM backend lsl the
> operands (shifts left) and compares the resulting operands (no right shift.)
> 
> For example, if an operand is an i8, the lowering sequence would be:
> 
> mov T0, Src0
> mov T1, Src1
> lsl T0, T0, 24
> cmp T0, T1 lsl 24
> 
> notice that by shifting the operand left by 24 bits, the operands most
> significant bit (the sign bit) ends up in bit 32 of the destination register.
> 
> I think MIPS could do something similar (i.e., simply left shift the
operands.)
> What do you think? For the signed case this could save two instructions per
> comparison.

Yes, I agree with this approach. Updated the patch to reflect the same. Thanks.

https://codereview.chromium.org/2022063003/diff/1/tests_lit/llvm2ice_tests/ic...
File tests_lit/llvm2ice_tests/icmp.ll (right):

https://codereview.chromium.org/2022063003/diff/1/tests_lit/llvm2ice_tests/ic...
tests_lit/llvm2ice_tests/icmp.ll:4: ; RUN:   --filetype=asm --target mips32 -i
%s --args -O2 --skip-unimplemented \
On 2016/05/31 19:00:20, stichnot wrote:
> Just a comment and an optional suggestion.
> 
> For an early-stage target lowering, and with filetype=asm, I like to lightly
try
> with both -Om1 and -O2.  Sometimes this reveals incorrect lowering and invalid
> addressing modes (leading to llvm-mc errors).  Sometimes "make check-lit" just
> works.  In other cases (including this one), pnacl-sz and llvm-mc generate
valid
> code, but FileCheck complains because the CHECK patterns don't match.
> 
> In this case, a cursory examination suggests that the failures are from
> overly-specific register assumptions.  It might be worth making the patterns
> more general, which could help future-proof the lit tests against future
> register allocation or related changes.

Done.

[John, 2016-06-01 14:57:52]

lgtm

[Jim Stichnoth, 2016-06-01 19:36:54]

Description was changed from

==========
[Subzero][MIPS32] Implement icmp operation for i8, i16 operands

BUG=none
==========

to

==========
[Subzero][MIPS32] Implement icmp operation for i8, i16 operands

BUG=none
R=jpp@chromium.org

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

[Jim Stichnoth, 2016-06-01 19:36:56]

Committed patchset #2 (id:20001) manually as
a393fd4027398e6cc15680aa1ba614540765eb34 (presubmit successful).