[SubZero] Implement GP to/from FP moves for MIPS

[SubZero] Implement GP to/from FP moves for MIPS

The patch implements mtc1/mfc1 instructions which are required for GP to/from FP registers moves.
The patch also implements fptosi and sitofp for float and i32 types to test mtc1/mfc1 instructions.

R=stichnot@chromium.org

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

[jaydeep.patil, 2016-09-07 05:39:37]

Description was changed from

==========
[SubZero] Implement GP to/from FP moves for MIPS

The patch implements mtc1/mfc1 instructions which are required for GP to/from FP
registers moves.
The patch also implements fptosi and sitofp for float and i32 types to test
mtc1/mfc1 instructions.
==========

to

==========
[SubZero] Implement GP to/from FP moves for MIPS

The patch implements mtc1/mfc1 instructions which are required for GP to/from FP
registers moves.
The patch also implements fptosi and sitofp for float and i32 types to test
mtc1/mfc1 instructions.
==========

[jaydeep.patil, 2016-09-07 05:39:38]

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

[Jim Stichnoth, 2016-09-07 15:39:32]

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:721: if (isScalarFloatingType(DstType) == true) {
remove the redundant "== true"

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:729: } else {
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

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

https://codereview.chromium.org/2316933002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1160: const bool isSrcGPReg =
RegMIPS32::isGPRReg(SrcR->getRegNum());
capitalize IsSrcGPReg per LLVM coding style

https://codereview.chromium.org/2316933002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2091: _mov(FTmp1, Src0R);
Is it really necessary to copy Src0R through a temporary?  Src0R is already
guaranteed to be in a register.  (This would be a performance issue, not a
correctness issue.)

https://codereview.chromium.org/2316933002/diff/1/tests_lit/llvm2ice_tests/fp...
File tests_lit/llvm2ice_tests/fp.convert.ll (right):

https://codereview.chromium.org/2316933002/diff/1/tests_lit/llvm2ice_tests/fp...
tests_lit/llvm2ice_tests/fp.convert.ll:19: ; RUN: %if --need=allow_dump
--need=target_MIPS32 --command %p2i --filetype=asm \
80-col

[obucinac, 2016-09-08 09:52:00]

obucinac@gmail.com changed reviewers:
+ obucinac@gmail.com

[obucinac, 2016-09-08 09:52:01]

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:744: ActualOpcode = "move";
As adviced earlier by Simon Dardis, we should use OR here.

Related links:

https://sourceware.org/ml/binutils/2015-07/msg00376.html

http://reviews.llvm.org/rL244579

Uncommited (but not commitable anymore) change:
https://codereview.chromium.org/2122043002/patch/40001/50001



Simons advice:

Your patch for supporting different data-types for Subzero has one small nit in
it:

+    case IceType_i1:
+    case IceType_i8:
+    case IceType_i16:
+    case IceType_i32:
+      Str << "\t"
+             "add"
              "\t";
-      getSrc(0)->emit(Func);
-      Str << ", ";
       getDest()->emit(Func);
+      Str << ", $zero, ";

That "add" should be an "or" instead. We re-encoded that macro to use 'or'
instead of 'add'
for all MIPS ISAs. See https://sourceware.org/ml/binutils/2015-07/msg00376.html
and
http://reviews.llvm.org/rL244579 .

The background for this is that 'or' is better than 'add' for 64bit cores
running O32 code as
'add' is required to sign-extend its result whereas 'or' doesn't.

Thanks,
Simon

[jaydeep.patil, 2016-09-13 06:18:36]

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:721: if (isScalarFloatingType(DstType) == true) {
On 2016/09/07 15:39:31, stichnot wrote:
> remove the redundant "== true"

Done.

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:729: } else {
On 2016/09/07 15:39:31, stichnot wrote:
> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

Done.

https://codereview.chromium.org/2316933002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:744: ActualOpcode = "move";
On 2016/09/08 09:52:01, obucinac wrote:
> As adviced earlier by Simon Dardis, we should use OR here.
> 
> Related links:
> 
> https://sourceware.org/ml/binutils/2015-07/msg00376.html
> 
> http://reviews.llvm.org/rL244579
> 
> Uncommited (but not commitable anymore) change:
> https://codereview.chromium.org/2122043002/patch/40001/50001
> 
> 
> 
> Simons advice:
> 
> Your patch for supporting different data-types for Subzero has one small nit
in
> it:
> 
> +    case IceType_i1:
> +    case IceType_i8:
> +    case IceType_i16:
> +    case IceType_i32:
> +      Str << "\t"
> +             "add"
>               "\t";
> -      getSrc(0)->emit(Func);
> -      Str << ", ";
>        getDest()->emit(Func);
> +      Str << ", $zero, ";
> 
> That "add" should be an "or" instead. We re-encoded that macro to use 'or'
> instead of 'add'
> for all MIPS ISAs. See
https://sourceware.org/ml/binutils/2015-07/msg00376.html
> and
> http://reviews.llvm.org/rL244579 .
> 
> The background for this is that 'or' is better than 'add' for 64bit cores
> running O32 code as
> 'add' is required to sign-extend its result whereas 'or' doesn't.
> 
> Thanks,
> Simon

This change should be done in AssemblerMIPS32::move instead of
InstMIPS32Mov::emitSingleDestSingleSource. 'move' is a pseudo instruction, let
the assembler encode it as 'or' instead of 'addu'.
We will submit a separate patch for this.

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

https://codereview.chromium.org/2316933002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:1160: const bool isSrcGPReg =
RegMIPS32::isGPRReg(SrcR->getRegNum());
On 2016/09/07 15:39:31, stichnot wrote:
> capitalize IsSrcGPReg per LLVM coding style

Done.

https://codereview.chromium.org/2316933002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:2091: _mov(FTmp1, Src0R);
On 2016/09/07 15:39:31, stichnot wrote:
> Is it really necessary to copy Src0R through a temporary?  Src0R is already
> guaranteed to be in a register.  (This would be a performance issue, not a
> correctness issue.)

Done.

https://codereview.chromium.org/2316933002/diff/1/tests_lit/llvm2ice_tests/fp...
File tests_lit/llvm2ice_tests/fp.convert.ll (right):

https://codereview.chromium.org/2316933002/diff/1/tests_lit/llvm2ice_tests/fp...
tests_lit/llvm2ice_tests/fp.convert.ll:19: ; RUN: %if --need=allow_dump
--need=target_MIPS32 --command %p2i --filetype=asm \
On 2016/09/07 15:39:32, stichnot wrote:
> 80-col

Done.

[Jim Stichnoth, 2016-09-13 13:05:37]

This looks good.

Could you rebase to master and upload a new patchset?  I'm getting merge
conflicts, which I can fix manually, but then I'm losing the correct "git cl"
commit message.

[obucinac, 2016-09-13 17:49:01]

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

https://codereview.chromium.org/2316933002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2086: case InstCast::Fptosi: {
Fptosi cast is covered by genTargetHelperCallSFor.

https://codereview.chromium.org/2316933002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2100: case InstCast::Sitofp: {
Sitofp cast is covered by genTargetHelperCallSFor.

[jaydeep.patil, 2016-09-14 04:42:44]

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

https://codereview.chromium.org/2316933002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2086: case InstCast::Fptosi: {
On 2016/09/13 17:49:01, obucinac wrote:
> Fptosi cast is covered by genTargetHelperCallSFor.

Done.

https://codereview.chromium.org/2316933002/diff/20001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:2100: case InstCast::Sitofp: {
On 2016/09/13 17:49:01, obucinac wrote:
> Sitofp cast is covered by genTargetHelperCallSFor.

Done.

[Jim Stichnoth, 2016-09-14 05:51:32]

lgtm

[Jim Stichnoth, 2016-09-14 05:52:29]

Description was changed from

==========
[SubZero] Implement GP to/from FP moves for MIPS

The patch implements mtc1/mfc1 instructions which are required for GP to/from FP
registers moves.
The patch also implements fptosi and sitofp for float and i32 types to test
mtc1/mfc1 instructions.
==========

to

==========
[SubZero] Implement GP to/from FP moves for MIPS

The patch implements mtc1/mfc1 instructions which are required for GP to/from FP
registers moves.
The patch also implements fptosi and sitofp for float and i32 types to test
mtc1/mfc1 instructions.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-09-14 05:52:30]

Committed patchset #3 (id:40001) manually as
d329766247adf63dbb8833a6819b2bdbbad6180b (presubmit successful).