Subzero, MIPS32: lowerSelect for i1, i8, i16, i32, f32, f64

Subzero, MIPS32: lowerSelect for i1, i8, i16, i32, f32, f64

Implements lowerSelect for basic integer and float datatypes.
Support for i64 will be added as separate patch.
Crucial for running crosstests.

R=stichnot@chromium.org

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

[obucinac, 2016-09-13 10:31:28]

Description was changed from

==========
Subzero, MIPS32: lowerSelect for i1, i8, i16, i32, f32, f64

Implements lowerSelect for basic integer and float datatypes.
Support for i64 will be added as separate patch.
Crucial for running crosstests.
==========

to

==========
Subzero, MIPS32: lowerSelect for i1, i8, i16, i32, f32, f64

Implements lowerSelect for basic integer and float datatypes.
Support for i64 will be added as separate patch.
Crucial for running crosstests.
==========

[obucinac, 2016-09-13 10:31:28]

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

[Jim Stichnoth, 2016-09-13 13:56:22]

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

https://codereview.chromium.org/2337023003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2734: case IceType_f32: {
What happens with an instruction like this?

  ret float 1.0
  ret double 1.0

Before, the code would do an UnimplementedLoweringError, which is fine.

Now, it looks like it will fall through to _ret(Reg_RA) which is surely wrong.

https://codereview.chromium.org/2337023003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2775: Type DestTy = Dest->getType();
const Type

https://codereview.chromium.org/2337023003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2795: _movn(SrcFR, SrcTR, ConditionR);
Have you tried running with "pnacl-sz -O2"?

I suspect these _movn instructions might trigger a liveness error, because SrcFR
is being redefined here (after being first defined above by copying the false
operand into a register).

If this happens, the way to deal with it is to add:
  _set_dest_redefined();

https://codereview.chromium.org/2337023003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2797: _mov(Dest, DestR);
Can you skip the previous _mov and just do
  _mov(Dest, SrcFR);
?

[obucinac, 2016-09-13 17:26:18]

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

https://codereview.chromium.org/2337023003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2734: case IceType_f32: {
On 2016/09/13 13:56:22, stichnot wrote:
> What happens with an instruction like this?
> 
>   ret float 1.0
>   ret double 1.0
> 
> Before, the code would do an UnimplementedLoweringError, which is fine.
> 
> Now, it looks like it will fall through to _ret(Reg_RA) which is surely wrong.

Done.

https://codereview.chromium.org/2337023003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2775: Type DestTy = Dest->getType();
On 2016/09/13 13:56:22, stichnot wrote:
> const Type

Done.

https://codereview.chromium.org/2337023003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2795: _movn(SrcFR, SrcTR, ConditionR);
On 2016/09/13 13:56:22, stichnot wrote:
> Have you tried running with "pnacl-sz -O2"?
> 
> I suspect these _movn instructions might trigger a liveness error, because
SrcFR
> is being redefined here (after being first defined above by copying the false
> operand into a register).
> 
> If this happens, the way to deal with it is to add:
>   _set_dest_redefined();

Checked, no problem here. O2 test added.

https://codereview.chromium.org/2337023003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2797: _mov(Dest, DestR);
On 2016/09/13 13:56:22, stichnot wrote:
> Can you skip the previous _mov and just do
>   _mov(Dest, SrcFR);
> ?

No. With previous changes, _mov handles only reg to reg copying. Without two
_mov we do not cover the case when destination is memory.

[Jim Stichnoth, 2016-09-14 06:02:18]

https://codereview.chromium.org/2337023003/diff/40001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2337023003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:3139: Operand *Src0F = legalize(Src0,
Legal_Reg);
legalizeToReg(Src0) ?

here and below

https://codereview.chromium.org/2337023003/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/select-opt.ll (right):

https://codereview.chromium.org/2337023003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/select-opt.ll:38: ; RUN:   --command FileCheck
--check-prefix MIPS32-O2 %s
It looks like the CHECK patterns are identical for Om1 and O2.

If this is true, and you expect it to continue to be true in the future, then
you could just "--check-prefix MIPS32" as above, and not have to duplicate the
CHECK patterns.

[edit: For testSelectImm32, I see there's a difference with addiu vs move, so
maybe my comment is irrelevant?  Or it could be matched using a regexp.]

https://codereview.chromium.org/2337023003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/select-opt.ll:101: ; MIPS32-O2-LABEL: testSelect
testSelectImm32

[obucinac, 2016-09-14 12:39:02]

https://codereview.chromium.org/2337023003/diff/40001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2337023003/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:3139: Operand *Src0F = legalize(Src0,
Legal_Reg);
On 2016/09/14 06:02:17, stichnot wrote:
> legalizeToReg(Src0) ?
> 
> here and below

Done.

https://codereview.chromium.org/2337023003/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/select-opt.ll (right):

https://codereview.chromium.org/2337023003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/select-opt.ll:38: ; RUN:   --command FileCheck
--check-prefix MIPS32-O2 %s
On 2016/09/14 06:02:18, stichnot wrote:
> It looks like the CHECK patterns are identical for Om1 and O2.
> 
> If this is true, and you expect it to continue to be true in the future, then
> you could just "--check-prefix MIPS32" as above, and not have to duplicate the
> CHECK patterns.
> 
> [edit: For testSelectImm32, I see there's a difference with addiu vs move, so
> maybe my comment is irrelevant?  Or it could be matched using a regexp.]

Done.

https://codereview.chromium.org/2337023003/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/select-opt.ll:101: ; MIPS32-O2-LABEL: testSelect
On 2016/09/14 06:02:17, stichnot wrote:
> testSelectImm32

Done.

https://codereview.chromium.org/2337023003/diff/60001/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2337023003/diff/60001/src/IceInstMIPS32.cpp#n...
src/IceInstMIPS32.cpp:747: (isScalarFloatingType(DstType) &&
isScalarIntegerType(SrcType))) {
e.g. IceType_i8 != IceType_i32, triggered emmiting mfc1/mtc1 instructions,
causing invalid operand error.

[Jim Stichnoth, 2016-09-14 13:06:17]

lgtm

[Jim Stichnoth, 2016-09-14 13:06:26]

Description was changed from

==========
Subzero, MIPS32: lowerSelect for i1, i8, i16, i32, f32, f64

Implements lowerSelect for basic integer and float datatypes.
Support for i64 will be added as separate patch.
Crucial for running crosstests.
==========

to

==========
Subzero, MIPS32: lowerSelect for i1, i8, i16, i32, f32, f64

Implements lowerSelect for basic integer and float datatypes.
Support for i64 will be added as separate patch.
Crucial for running crosstests.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-09-14 13:06:28]

Committed patchset #4 (id:60001) manually as
d57ed5fabb019ec0a29961f3a45bf82437e66bbe (presubmit successful).