[Qemu-devel] [PATCH] mips: Fix the 64-bit case for microMIPS MOVE16 and MOVEP

[Qemu-devel] [PATCH] mips: Fix the 64-bit case for microMIPS MOVE16 and MOVEP

[Maciej W. Rozycki]

Fix microMIPS MOVE16 and MOVEP instructions on 64-bit processors by 
using register addition operations.

This copies the approach taken with MIPS16 MOVE instructions (I8_MOV32R 
and I8_MOVR32 opcodes) and follows the observation that OPC_ADDU expands 
to tcg_gen_mov_tl whenever `rt' is 0 and `rs' is not, therefore copying 
`rs' to `rd' verbatim.  This is not the case with OPC_ADDIU where a 
sign-extension from bit #31 is made, unless in the uninteresting case of 
`rs' being 0, losing the upper 32 bits of the value copied for any 
proper 64-bit values.

This also serves as an optimization as one op is produced in generated 
code rather than two (again, unless `rs' is 0, where it doesn't change 
anything).

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---
 This is rather obvious, but I also pushed it through full bare-iron GCC 
regression testing with an o32 big-endian microMIPS multilib.  That 
includes several instances of both instructions.  No changes in results 
were observed with the patch applied compared to the original version.

 I wonder if all these move operations shouldn't actually be switched to 
using OPC_OR that is agnostic to the machine word size regardless of the 
operand selection.  But that is something to consider separately.

 So meanwhile, please apply.

  Maciej

qemu-umips-move.diff
Index: qemu-git-trunk/target-mips/translate.c
===================================================================
--- qemu-git-trunk.orig/target-mips/translate.c	2014-11-02 17:57:16.998924336 +0000
+++ qemu-git-trunk/target-mips/translate.c	2014-11-02 17:57:19.498930155 +0000
@@ -13492,8 +13492,8 @@ static int decode_micromips_opc (CPUMIPS
             rs = rs_rt_enc[enc_rs];
             rt = rs_rt_enc[enc_rt];
 
-            gen_arith_imm(ctx, OPC_ADDIU, rd, rs, 0);
-            gen_arith_imm(ctx, OPC_ADDIU, re, rt, 0);
+            gen_arith(ctx, OPC_ADDU, rd, rs, 0);
+            gen_arith(ctx, OPC_ADDU, re, rt, 0);
         }
         break;
     case LBU16:
@@ -13574,7 +13574,7 @@ static int decode_micromips_opc (CPUMIPS
             int rd = uMIPS_RD5(ctx->opcode);
             int rs = uMIPS_RS5(ctx->opcode);
 
-            gen_arith_imm(ctx, OPC_ADDIU, rd, rs, 0);
+            gen_arith(ctx, OPC_ADDU, rd, rs, 0);
         }
         break;
     case ANDI16:

Re: [Qemu-devel] [PATCH] mips: Fix the 64-bit case for microMIPS MOVE16 and MOVEP

[Andreas Färber]

Hi Maciej,

Please consistently use "target-mips: " when that's what you're
touching. (For hw/mips/ it's less consistent what to use.)

Leon, please sanitize subjects before sending them out, it makes them
easier to skim in git-log and cgit.

Thanks,
Andreas

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg

Re: [Qemu-devel] [PATCH] mips: Fix the 64-bit case for microMIPS MOVE16 and MOVEP

[Maciej W. Rozycki]

On Wed, 12 Nov 2014, Andreas Färber wrote:

> Please consistently use "target-mips: " when that's what you're
> touching. (For hw/mips/ it's less consistent what to use.)

 Sure.  What about MIPS changes that span files contained within 
target-mips/ and elsewhere?  I have such changes in my queue.

  Maciej

Re: [Qemu-devel] [PATCH] mips: Fix the 64-bit case for microMIPS MOVE16 and MOVEP

[Leon Alrae]

On 12/11/2014 18:46, Maciej W. Rozycki wrote:
> On Wed, 12 Nov 2014, Andreas Färber wrote:
> 
>> Please consistently use "target-mips: " when that's what you're
>> touching. (For hw/mips/ it's less consistent what to use.)
> 
>  Sure.  What about MIPS changes that span files contained within 
> target-mips/ and elsewhere?  I have such changes in my queue.

It might be a good idea to split these changes into separate patches to
have more precise indication about touched subsystem (even though all
the changes were done in MIPS context). For example "target-mips" and
"linux-user" rather than just "mips".

Regards,
Leon

Re: [Qemu-devel] [PATCH] mips: Fix the 64-bit case for microMIPS MOVE16 and MOVEP

[Maciej W. Rozycki]

On Thu, 13 Nov 2014, Leon Alrae wrote:

> It might be a good idea to split these changes into separate patches to
> have more precise indication about touched subsystem (even though all
> the changes were done in MIPS context). For example "target-mips" and
> "linux-user" rather than just "mips".

 I split changes as applicable wherever I can, but at least some of 
these changes cannot be split without losing the sense of the change or 
even without causing compilation breakage.  Well, I plan to submit those 
last, so it'll be a while yet and we can think about it when the time 
comes.  Thanks for your input.

  Maciej

Re: [Qemu-devel] [PATCH] mips: Fix the 64-bit case for microMIPS MOVE16 and MOVEP

[Leon Alrae]

On 12/11/2014 15:21, Maciej W. Rozycki wrote:
> Fix microMIPS MOVE16 and MOVEP instructions on 64-bit processors by 
> using register addition operations.
> 
> This copies the approach taken with MIPS16 MOVE instructions (I8_MOV32R 
> and I8_MOVR32 opcodes) and follows the observation that OPC_ADDU expands 
> to tcg_gen_mov_tl whenever `rt' is 0 and `rs' is not, therefore copying 
> `rs' to `rd' verbatim.  This is not the case with OPC_ADDIU where a 
> sign-extension from bit #31 is made, unless in the uninteresting case of 
> `rs' being 0, losing the upper 32 bits of the value copied for any 
> proper 64-bit values.
> 
> This also serves as an optimization as one op is produced in generated 
> code rather than two (again, unless `rs' is 0, where it doesn't change 
> anything).
> 
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> ---
>  This is rather obvious, but I also pushed it through full bare-iron GCC 
> regression testing with an o32 big-endian microMIPS multilib.  That 
> includes several instances of both instructions.  No changes in results 
> were observed with the patch applied compared to the original version.
> 
>  I wonder if all these move operations shouldn't actually be switched to 
> using OPC_OR that is agnostic to the machine word size regardless of the 
> operand selection.  But that is something to consider separately.
> 
>  So meanwhile, please apply.
> 
>   Maciej
> 
> qemu-umips-move.diff
> Index: qemu-git-trunk/target-mips/translate.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/translate.c	2014-11-02 17:57:16.998924336 +0000
> +++ qemu-git-trunk/target-mips/translate.c	2014-11-02 17:57:19.498930155 +0000
> @@ -13492,8 +13492,8 @@ static int decode_micromips_opc (CPUMIPS
>              rs = rs_rt_enc[enc_rs];
>              rt = rs_rt_enc[enc_rt];
>  
> -            gen_arith_imm(ctx, OPC_ADDIU, rd, rs, 0);
> -            gen_arith_imm(ctx, OPC_ADDIU, re, rt, 0);
> +            gen_arith(ctx, OPC_ADDU, rd, rs, 0);
> +            gen_arith(ctx, OPC_ADDU, re, rt, 0);
>          }
>          break;
>      case LBU16:
> @@ -13574,7 +13574,7 @@ static int decode_micromips_opc (CPUMIPS
>              int rd = uMIPS_RD5(ctx->opcode);
>              int rs = uMIPS_RS5(ctx->opcode);
>  
> -            gen_arith_imm(ctx, OPC_ADDIU, rd, rs, 0);
> +            gen_arith(ctx, OPC_ADDU, rd, rs, 0);
>          }
>          break;
>      case ANDI16:

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>

All the patches up to this one have been applied to mips-next branch
(available at git://github.com/lalrae/qemu.git), thanks. I'll go through
the remaining soon.

Regards,
Leon

Re: [Qemu-devel] [PATCH] mips: Fix the 64-bit case for microMIPS MOVE16 and MOVEP

[Maciej W. Rozycki]

On Mon, 24 Nov 2014, Leon Alrae wrote:

> All the patches up to this one have been applied to mips-next branch
> (available at git://github.com/lalrae/qemu.git), thanks. I'll go through
> the remaining soon.

 Thanks.  I am now back from a week's vacation and will continue posting 
outstanding changes.

 There will be changes made to generic code, specifically soft-float, 
and consequently other platforms' code, to suit the MIPS implementation 
of IEEE 754-2008 NaN handling recommendation.  Regrettably I see the 
delivery of the 2.2 release has been delayed and I do hope the new 
estimate of Dec 5th will stand so that the changes can make their way to 
trunk in a timely manner.

 I have some other stuff beyond 2008-NaN support as well, but I'll be 
giving the latter a priority as I know you look forward to seeing it and 
the rest is valuable, but a bit less important (and furthermore it 
relies on some changes to ABI configuration that we may need to discuss 
before we find a satisfactory solution).

  Maciej