diff mbox series

[2/7] target/mips: Add MXU instructions S32I2M and S32M2I

Message ID f1e92127959ffb3df713a06452328a71170bda93.1535133089.git.jancraig@amazon.com (mailing list archive)
State New, archived
Headers show
Series Add limited MXU instruction support | expand

Commit Message

Denis V. Lunev" via Aug. 24, 2018, 7:44 p.m. UTC
Adds support for emulating the S32I2M and S32M2I MXU instructions.

Signed-off-by: Craig Janeczek <jancraig@amazon.com>
---
 target/mips/translate.c | 55 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Richard Henderson Aug. 25, 2018, 5:07 p.m. UTC | #1
On 08/24/2018 12:44 PM, Craig Janeczek via Qemu-devel wrote:
> Adds support for emulating the S32I2M and S32M2I MXU instructions.
> 
> Signed-off-by: Craig Janeczek <jancraig@amazon.com>
> ---
>  target/mips/translate.c | 55 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 50f0cb558f..381dfad36e 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -364,6 +364,9 @@ enum {
>      OPC_CLO      = 0x21 | OPC_SPECIAL2,
>      OPC_DCLZ     = 0x24 | OPC_SPECIAL2,
>      OPC_DCLO     = 0x25 | OPC_SPECIAL2,
> +    /* MXU */
> +    OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
> +    OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,

I haven't been able to find any documentation of the bit
layout of these instructions.  Any pointers?

> +typedef union {
> +    struct {
> +        uint32_t op:6;
> +        uint32_t xra:5;
> +        uint32_t:5;
> +        uint32_t rb:5;
> +        uint32_t:5;
> +        uint32_t special2:6;
> +    } S32I2M;
> +
> +    struct {
> +        uint32_t op:6;
> +        uint32_t xra:5;
> +        uint32_t:5;
> +        uint32_t rb:5;
> +        uint32_t:5;
> +        uint32_t special2:6;
> +    } S32M2I;
> +} MXU_OPCODE;

Do not use bitfields.  The layout differs by host compiler.
Use extract32(input, pos, len).


> +
> +/* MXU Instructions */
> +static void gen_mxu(DisasContext *ctx, uint32_t opc)
> +{
> +#ifndef TARGET_MIPS64 /* Only works in 32 bit mode */
> +    TCGv t0;
> +    t0 = tcg_temp_new();
> +    MXU_OPCODE *opcode = (MXU_OPCODE *)&ctx->opcode;
> +
> +    switch (opc) {
> +    case OPC_MXU_S32I2M:
> +        gen_load_gpr(t0, opcode->S32I2M.rb);
> +        gen_store_mxu_gpr(t0, opcode->S32I2M.xra);
> +        break;
> +
> +    case OPC_MXU_S32M2I:
> +        gen_load_mxu_gpr(t0, opcode->S32M2I.xra);
> +        gen_store_gpr(t0, opcode->S32M2I.rb);
> +        break;
> +    }
> +
> +    tcg_temp_free(t0);
> +#else
> +    generate_exception_end(ctx, EXCP_RI);
> +#endif
> +}

There's nothing here (yet, I suppose) that won't compile for MIPS64.
I'd suggest avoiding ifdefs as much as possible.


r~
Denis V. Lunev" via Aug. 27, 2018, 12:14 p.m. UTC | #2
https://www.rockbox.org/wiki/pub/Main/IngenicJz4760B/jz-simd-docs.pdf

I pulled them from here. I also wrote a series of tests which I cross compiled then ran on both HW and through QEMU. Although I did not submit those tests as part of this patchset as I am unsure of how to add them into the QEMU test infrastructure.

-----Original Message-----
From: Richard Henderson <richard.henderson@linaro.org> 
Sent: Saturday, August 25, 2018 1:07 PM
To: Janeczek, Craig <jancraig@amazon.com>; qemu-devel@nongnu.org
Cc: aurelien@aurel32.net; amarkovic@wavecomp.com
Subject: Re: [Qemu-devel] [PATCH 2/7] target/mips: Add MXU instructions S32I2M and S32M2I

On 08/24/2018 12:44 PM, Craig Janeczek via Qemu-devel wrote:
> Adds support for emulating the S32I2M and S32M2I MXU instructions.
> 
> Signed-off-by: Craig Janeczek <jancraig@amazon.com>
> ---
>  target/mips/translate.c | 55 
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c index 
> 50f0cb558f..381dfad36e 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -364,6 +364,9 @@ enum {
>      OPC_CLO      = 0x21 | OPC_SPECIAL2,
>      OPC_DCLZ     = 0x24 | OPC_SPECIAL2,
>      OPC_DCLO     = 0x25 | OPC_SPECIAL2,
> +    /* MXU */
> +    OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
> +    OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,

I haven't been able to find any documentation of the bit layout of these instructions.  Any pointers?

> +typedef union {
> +    struct {
> +        uint32_t op:6;
> +        uint32_t xra:5;
> +        uint32_t:5;
> +        uint32_t rb:5;
> +        uint32_t:5;
> +        uint32_t special2:6;
> +    } S32I2M;
> +
> +    struct {
> +        uint32_t op:6;
> +        uint32_t xra:5;
> +        uint32_t:5;
> +        uint32_t rb:5;
> +        uint32_t:5;
> +        uint32_t special2:6;
> +    } S32M2I;
> +} MXU_OPCODE;

Do not use bitfields.  The layout differs by host compiler.
Use extract32(input, pos, len).


> +
> +/* MXU Instructions */
> +static void gen_mxu(DisasContext *ctx, uint32_t opc) { #ifndef 
> +TARGET_MIPS64 /* Only works in 32 bit mode */
> +    TCGv t0;
> +    t0 = tcg_temp_new();
> +    MXU_OPCODE *opcode = (MXU_OPCODE *)&ctx->opcode;
> +
> +    switch (opc) {
> +    case OPC_MXU_S32I2M:
> +        gen_load_gpr(t0, opcode->S32I2M.rb);
> +        gen_store_mxu_gpr(t0, opcode->S32I2M.xra);
> +        break;
> +
> +    case OPC_MXU_S32M2I:
> +        gen_load_mxu_gpr(t0, opcode->S32M2I.xra);
> +        gen_store_gpr(t0, opcode->S32M2I.rb);
> +        break;
> +    }
> +
> +    tcg_temp_free(t0);
> +#else
> +    generate_exception_end(ctx, EXCP_RI); #endif }

There's nothing here (yet, I suppose) that won't compile for MIPS64.
I'd suggest avoiding ifdefs as much as possible.


r~
Denis V. Lunev" via Aug. 27, 2018, 12:22 p.m. UTC | #3
https://github.com/MIPS/CI20_mplayer/blob/ci20_mplayer/mxu_as

Sorry I mis-read our comment. The bit layouts were pulled from this script and validated by visually examining compiled code.

-----Original Message-----
From: Janeczek, Craig 
Sent: Monday, August 27, 2018 8:15 AM
To: 'Richard Henderson' <richard.henderson@linaro.org>; qemu-devel@nongnu.org
Cc: aurelien@aurel32.net; amarkovic@wavecomp.com
Subject: RE: [Qemu-devel] [PATCH 2/7] target/mips: Add MXU instructions S32I2M and S32M2I

https://www.rockbox.org/wiki/pub/Main/IngenicJz4760B/jz-simd-docs.pdf

I pulled them from here. I also wrote a series of tests which I cross compiled then ran on both HW and through QEMU. Although I did not submit those tests as part of this patchset as I am unsure of how to add them into the QEMU test infrastructure.

-----Original Message-----
From: Richard Henderson <richard.henderson@linaro.org>
Sent: Saturday, August 25, 2018 1:07 PM
To: Janeczek, Craig <jancraig@amazon.com>; qemu-devel@nongnu.org
Cc: aurelien@aurel32.net; amarkovic@wavecomp.com
Subject: Re: [Qemu-devel] [PATCH 2/7] target/mips: Add MXU instructions S32I2M and S32M2I

On 08/24/2018 12:44 PM, Craig Janeczek via Qemu-devel wrote:
> Adds support for emulating the S32I2M and S32M2I MXU instructions.
> 
> Signed-off-by: Craig Janeczek <jancraig@amazon.com>
> ---
>  target/mips/translate.c | 55
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c index 
> 50f0cb558f..381dfad36e 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -364,6 +364,9 @@ enum {
>      OPC_CLO      = 0x21 | OPC_SPECIAL2,
>      OPC_DCLZ     = 0x24 | OPC_SPECIAL2,
>      OPC_DCLO     = 0x25 | OPC_SPECIAL2,
> +    /* MXU */
> +    OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
> +    OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,

I haven't been able to find any documentation of the bit layout of these instructions.  Any pointers?

> +typedef union {
> +    struct {
> +        uint32_t op:6;
> +        uint32_t xra:5;
> +        uint32_t:5;
> +        uint32_t rb:5;
> +        uint32_t:5;
> +        uint32_t special2:6;
> +    } S32I2M;
> +
> +    struct {
> +        uint32_t op:6;
> +        uint32_t xra:5;
> +        uint32_t:5;
> +        uint32_t rb:5;
> +        uint32_t:5;
> +        uint32_t special2:6;
> +    } S32M2I;
> +} MXU_OPCODE;

Do not use bitfields.  The layout differs by host compiler.
Use extract32(input, pos, len).


> +
> +/* MXU Instructions */
> +static void gen_mxu(DisasContext *ctx, uint32_t opc) { #ifndef
> +TARGET_MIPS64 /* Only works in 32 bit mode */
> +    TCGv t0;
> +    t0 = tcg_temp_new();
> +    MXU_OPCODE *opcode = (MXU_OPCODE *)&ctx->opcode;
> +
> +    switch (opc) {
> +    case OPC_MXU_S32I2M:
> +        gen_load_gpr(t0, opcode->S32I2M.rb);
> +        gen_store_mxu_gpr(t0, opcode->S32I2M.xra);
> +        break;
> +
> +    case OPC_MXU_S32M2I:
> +        gen_load_mxu_gpr(t0, opcode->S32M2I.xra);
> +        gen_store_gpr(t0, opcode->S32M2I.rb);
> +        break;
> +    }
> +
> +    tcg_temp_free(t0);
> +#else
> +    generate_exception_end(ctx, EXCP_RI); #endif }

There's nothing here (yet, I suppose) that won't compile for MIPS64.
I'd suggest avoiding ifdefs as much as possible.


r~
Aleksandar Markovic Aug. 27, 2018, 1:21 p.m. UTC | #4
> Craig Janeczek via Qemu-devel wrote:
>
> I also wrote a series of tests which I cross compiled then ran on
> both HW and through QEMU. Although I did not submit those tests
> as part of this patchset as I am unsure of how to add them into the 
> QEMU test infrastructure.

Alex,

Here we have another case of someone having - and wanting to add to QEMU - some MIPS tests.

I gather they are very similar to the tests in:

tests/tcg/mips/mips32-dsp, and
tests/tcg/mips/mips32-dspr2.

However, you say something is wrong with them (but we are using them for testings of DSP and DSP R2 ASE for MIPS CPUs that have such feature).

Can you please tell us:

- What should we do to existing DSP and DSP R2 to fit into QEMU test infrastructure? 

- What would be the guidance for Craig to integrate new tests that he wrote into QEMU (which are I suppose of similar nature to DSP tests)?

Many thanks!

Aleksandar
Aleksandar Markovic Aug. 27, 2018, 1:25 p.m. UTC | #5
> From: Janeczek, Craig <jancraig@amazon.com>
> Sent: Monday, August 27, 2018 2:22 PM
> 
> Subject: RE: [Qemu-devel] [PATCH 2/7] target/mips: Add MXU instructions S32I2M and S32M2I
> 
> https://github.com/MIPS/CI20_mplayer/blob/ci20_mplayer/mxu_as
> 
> Sorry I mis-read our comment. The bit layouts were pulled from
> this script and validated by visually examining compiled code.

I think Richard brings to your attention that the structure definitions using bit fields may yield different results on little and big endian hosts, which is certainly the situation you want to avooid, and recommends using extract32(), or similar QEMU feature, instead.

Aleksandar
diff mbox series

Patch

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 50f0cb558f..381dfad36e 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -364,6 +364,9 @@  enum {
     OPC_CLO      = 0x21 | OPC_SPECIAL2,
     OPC_DCLZ     = 0x24 | OPC_SPECIAL2,
     OPC_DCLO     = 0x25 | OPC_SPECIAL2,
+    /* MXU */
+    OPC_MXU_S32I2M = 0x2F | OPC_SPECIAL2,
+    OPC_MXU_S32M2I = 0x2E | OPC_SPECIAL2,
     /* Special */
     OPC_SDBBP    = 0x3F | OPC_SPECIAL2,
 };
@@ -3763,6 +3766,52 @@  static void gen_cl (DisasContext *ctx, uint32_t opc,
     }
 }
 
+typedef union {
+    struct {
+        uint32_t op:6;
+        uint32_t xra:5;
+        uint32_t:5;
+        uint32_t rb:5;
+        uint32_t:5;
+        uint32_t special2:6;
+    } S32I2M;
+
+    struct {
+        uint32_t op:6;
+        uint32_t xra:5;
+        uint32_t:5;
+        uint32_t rb:5;
+        uint32_t:5;
+        uint32_t special2:6;
+    } S32M2I;
+} MXU_OPCODE;
+
+/* MXU Instructions */
+static void gen_mxu(DisasContext *ctx, uint32_t opc)
+{
+#ifndef TARGET_MIPS64 /* Only works in 32 bit mode */
+    TCGv t0;
+    t0 = tcg_temp_new();
+    MXU_OPCODE *opcode = (MXU_OPCODE *)&ctx->opcode;
+
+    switch (opc) {
+    case OPC_MXU_S32I2M:
+        gen_load_gpr(t0, opcode->S32I2M.rb);
+        gen_store_mxu_gpr(t0, opcode->S32I2M.xra);
+        break;
+
+    case OPC_MXU_S32M2I:
+        gen_load_mxu_gpr(t0, opcode->S32M2I.xra);
+        gen_store_gpr(t0, opcode->S32M2I.rb);
+        break;
+    }
+
+    tcg_temp_free(t0);
+#else
+    generate_exception_end(ctx, EXCP_RI);
+#endif
+}
+
 /* Godson integer instructions */
 static void gen_loongson_integer(DisasContext *ctx, uint32_t opc,
                                  int rd, int rs, int rt)
@@ -17843,6 +17892,12 @@  static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx)
         check_insn(ctx, INSN_LOONGSON2F);
         gen_loongson_integer(ctx, op1, rd, rs, rt);
         break;
+
+    case OPC_MXU_S32I2M:
+    case OPC_MXU_S32M2I:
+        gen_mxu(ctx, op1);
+        break;
+
     case OPC_CLO:
     case OPC_CLZ:
         check_insn(ctx, ISA_MIPS32);