diff mbox series

[v5,2/8] target/mips: Support R5900 specific three-operand MULT and MULTU

Message ID fd1b1f640912284963bb1d9974a80f35cc1d4a72.1537379317.git.noring@nocrew.org
State New
Headers show
Series target/mips: Support R5900 GCC programs in user mode | expand

Commit Message

Fredrik Noring Sept. 15, 2018, 9:25 a.m. UTC 
Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
 target/mips/translate.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Philippe Mathieu-Daudé Sept. 25, 2018, 12:20 p.m. UTC | #1 
Hi Fredrik,

On 9/15/18 11:25 AM, Fredrik Noring wrote:
> Signed-off-by: Fredrik Noring <noring@nocrew.org>
> ---
>  target/mips/translate.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index ab16cdb911..fb571e278e 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -3768,6 +3768,57 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
>      tcg_temp_free(t1);
>  }
>  
> +static void gen_mul_r5900(DisasContext *ctx, uint32_t opc,
> +                          int acc, int rd, int rs, int rt)
> +{
> +    TCGv t0 = tcg_temp_new();
> +    TCGv t1 = tcg_temp_new();
> +
> +    gen_load_gpr(t0, rs);
> +    gen_load_gpr(t1, rt);
> +
> +    switch (opc) {
> +    case OPC_MULT:
> +        {
> +            TCGv_i32 t2 = tcg_temp_new_i32();
> +            TCGv_i32 t3 = tcg_temp_new_i32();
> +            tcg_gen_trunc_tl_i32(t2, t0);
> +            tcg_gen_trunc_tl_i32(t3, t1);
> +            tcg_gen_muls2_i32(t2, t3, t2, t3);
> +            if (rd)
> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
> +            tcg_temp_free_i32(t2);
> +            tcg_temp_free_i32(t3);
> +        }
> +        break;
> +    case OPC_MULTU:
> +        {
> +            TCGv_i32 t2 = tcg_temp_new_i32();
> +            TCGv_i32 t3 = tcg_temp_new_i32();
> +            tcg_gen_trunc_tl_i32(t2, t0);
> +            tcg_gen_trunc_tl_i32(t3, t1);
> +            tcg_gen_mulu2_i32(t2, t3, t2, t3);
> +            if (rd)
> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
> +            tcg_temp_free_i32(t2);
> +            tcg_temp_free_i32(t3);
> +        }
> +        break;
> +    default:
> +        MIPS_INVAL("mul R5900");
> +        generate_exception_end(ctx, EXCP_RI);
> +        goto out;
> +    }
> +
> + out:
> +    tcg_temp_free(t0);
> +    tcg_temp_free(t1);
> +}

Why not simply modify gen_muldiv?

-- >8 --
@@ -3630,29 +3630,35 @@ static void gen_muldiv(DisasContext *ctx,
uint32_t opc,
         break;
     case OPC_MULT:
         {
             TCGv_i32 t2 = tcg_temp_new_i32();
             TCGv_i32 t3 = tcg_temp_new_i32();
             tcg_gen_trunc_tl_i32(t2, t0);
             tcg_gen_trunc_tl_i32(t3, t1);
             tcg_gen_muls2_i32(t2, t3, t2, t3);
+            if (ctx->insn_flags & INSN_R5900 && rd) {
+                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
+            }
             tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
             tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
             tcg_temp_free_i32(t2);
             tcg_temp_free_i32(t3);
         }
         break;
     case OPC_MULTU:
         {
             TCGv_i32 t2 = tcg_temp_new_i32();
             TCGv_i32 t3 = tcg_temp_new_i32();
             tcg_gen_trunc_tl_i32(t2, t0);
             tcg_gen_trunc_tl_i32(t3, t1);
             tcg_gen_mulu2_i32(t2, t3, t2, t3);
+            if (ctx->insn_flags & INSN_R5900 && rd) {
+                tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
+            }
             tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
             tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
             tcg_temp_free_i32(t2);
             tcg_temp_free_i32(t3);
         }
         break;
---

> +
>  static void gen_mul_vr54xx (DisasContext *ctx, uint32_t opc,
>                              int rd, int rs, int rt)
>  {
> @@ -22378,6 +22429,8 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
>              check_insn(ctx, INSN_VR54XX);
>              op1 = MASK_MUL_VR54XX(ctx->opcode);
>              gen_mul_vr54xx(ctx, op1, rd, rs, rt);
> +        } else if (ctx->insn_flags & INSN_R5900) {
> +            gen_mul_r5900(ctx, op1, 0, rd, rs, rt);
>          } else {
>              gen_muldiv(ctx, op1, rd & 3, rs, rt);
>          }
>
Philippe Mathieu-Daudé Sept. 26, 2018, 9:59 p.m. UTC | #2 
On 9/25/18 2:20 PM, Philippe Mathieu-Daudé wrote:
> Hi Fredrik,
> 
> On 9/15/18 11:25 AM, Fredrik Noring wrote:
>> Signed-off-by: Fredrik Noring <noring@nocrew.org>
>> ---
>>  target/mips/translate.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/target/mips/translate.c b/target/mips/translate.c
>> index ab16cdb911..fb571e278e 100644
>> --- a/target/mips/translate.c
>> +++ b/target/mips/translate.c
>> @@ -3768,6 +3768,57 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
>>      tcg_temp_free(t1);
>>  }
>>  
>> +static void gen_mul_r5900(DisasContext *ctx, uint32_t opc,
>> +                          int acc, int rd, int rs, int rt)
>> +{
>> +    TCGv t0 = tcg_temp_new();
>> +    TCGv t1 = tcg_temp_new();
>> +
>> +    gen_load_gpr(t0, rs);
>> +    gen_load_gpr(t1, rt);
>> +
>> +    switch (opc) {
>> +    case OPC_MULT:
>> +        {
>> +            TCGv_i32 t2 = tcg_temp_new_i32();
>> +            TCGv_i32 t3 = tcg_temp_new_i32();
>> +            tcg_gen_trunc_tl_i32(t2, t0);
>> +            tcg_gen_trunc_tl_i32(t3, t1);
>> +            tcg_gen_muls2_i32(t2, t3, t2, t3);
>> +            if (rd)
>> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
>> +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>> +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
>> +            tcg_temp_free_i32(t2);
>> +            tcg_temp_free_i32(t3);
>> +        }
>> +        break;
>> +    case OPC_MULTU:
>> +        {
>> +            TCGv_i32 t2 = tcg_temp_new_i32();
>> +            TCGv_i32 t3 = tcg_temp_new_i32();
>> +            tcg_gen_trunc_tl_i32(t2, t0);
>> +            tcg_gen_trunc_tl_i32(t3, t1);
>> +            tcg_gen_mulu2_i32(t2, t3, t2, t3);
>> +            if (rd)
>> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
>> +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>> +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
>> +            tcg_temp_free_i32(t2);
>> +            tcg_temp_free_i32(t3);
>> +        }
>> +        break;
>> +    default:
>> +        MIPS_INVAL("mul R5900");
>> +        generate_exception_end(ctx, EXCP_RI);
>> +        goto out;
>> +    }
>> +
>> + out:
>> +    tcg_temp_free(t0);
>> +    tcg_temp_free(t1);
>> +}
> 
> Why not simply modify gen_muldiv?

Testing my own patch eh...

I missed the check_dsp() call:

    if (acc != 0 && unlikely(!(ctx->insn_flags & INSN_R5900))) {
        check_dsp(ctx);
    }

Anyway if we plan to also add MADD/MADDU, using another function might
be cleaner.

> 
> -- >8 --
> @@ -3630,29 +3630,35 @@ static void gen_muldiv(DisasContext *ctx,
> uint32_t opc,
>          break;
>      case OPC_MULT:
>          {
>              TCGv_i32 t2 = tcg_temp_new_i32();
>              TCGv_i32 t3 = tcg_temp_new_i32();
>              tcg_gen_trunc_tl_i32(t2, t0);
>              tcg_gen_trunc_tl_i32(t3, t1);
>              tcg_gen_muls2_i32(t2, t3, t2, t3);
> +            if (ctx->insn_flags & INSN_R5900 && rd) {
> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> +            }
>              tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>              tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
>              tcg_temp_free_i32(t2);
>              tcg_temp_free_i32(t3);
>          }
>          break;
>      case OPC_MULTU:
>          {
>              TCGv_i32 t2 = tcg_temp_new_i32();
>              TCGv_i32 t3 = tcg_temp_new_i32();
>              tcg_gen_trunc_tl_i32(t2, t0);
>              tcg_gen_trunc_tl_i32(t3, t1);
>              tcg_gen_mulu2_i32(t2, t3, t2, t3);
> +            if (ctx->insn_flags & INSN_R5900 && rd) {
> +                tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> +            }
>              tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>              tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
>              tcg_temp_free_i32(t2);
>              tcg_temp_free_i32(t3);
>          }
>          break;
> ---
> 
>> +
>>  static void gen_mul_vr54xx (DisasContext *ctx, uint32_t opc,
>>                              int rd, int rs, int rt)
>>  {
>> @@ -22378,6 +22429,8 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
>>              check_insn(ctx, INSN_VR54XX);
>>              op1 = MASK_MUL_VR54XX(ctx->opcode);
>>              gen_mul_vr54xx(ctx, op1, rd, rs, rt);
>> +        } else if (ctx->insn_flags & INSN_R5900) {
>> +            gen_mul_r5900(ctx, op1, 0, rd, rs, rt);
>>          } else {
>>              gen_muldiv(ctx, op1, rd & 3, rs, rt);
>>          }
>>
Philippe Mathieu-Daudé Sept. 26, 2018, 10:06 p.m. UTC | #3 
On 9/26/18 11:59 PM, Philippe Mathieu-Daudé wrote:
> On 9/25/18 2:20 PM, Philippe Mathieu-Daudé wrote:
>> Hi Fredrik,
>>
>> On 9/15/18 11:25 AM, Fredrik Noring wrote:
>>> Signed-off-by: Fredrik Noring <noring@nocrew.org>
>>> ---
>>>  target/mips/translate.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 53 insertions(+)
>>>
>>> diff --git a/target/mips/translate.c b/target/mips/translate.c
>>> index ab16cdb911..fb571e278e 100644
>>> --- a/target/mips/translate.c
>>> +++ b/target/mips/translate.c
>>> @@ -3768,6 +3768,57 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
>>>      tcg_temp_free(t1);
>>>  }
>>>  
>>> +static void gen_mul_r5900(DisasContext *ctx, uint32_t opc,
>>> +                          int acc, int rd, int rs, int rt)
>>> +{
>>> +    TCGv t0 = tcg_temp_new();
>>> +    TCGv t1 = tcg_temp_new();
>>> +
>>> +    gen_load_gpr(t0, rs);
>>> +    gen_load_gpr(t1, rt);
>>> +
>>> +    switch (opc) {
>>> +    case OPC_MULT:
>>> +        {
>>> +            TCGv_i32 t2 = tcg_temp_new_i32();
>>> +            TCGv_i32 t3 = tcg_temp_new_i32();
>>> +            tcg_gen_trunc_tl_i32(t2, t0);
>>> +            tcg_gen_trunc_tl_i32(t3, t1);
>>> +            tcg_gen_muls2_i32(t2, t3, t2, t3);
>>> +            if (rd)
>>> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
>>> +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>>> +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
>>> +            tcg_temp_free_i32(t2);
>>> +            tcg_temp_free_i32(t3);
>>> +        }
>>> +        break;
>>> +    case OPC_MULTU:
>>> +        {
>>> +            TCGv_i32 t2 = tcg_temp_new_i32();
>>> +            TCGv_i32 t3 = tcg_temp_new_i32();
>>> +            tcg_gen_trunc_tl_i32(t2, t0);
>>> +            tcg_gen_trunc_tl_i32(t3, t1);
>>> +            tcg_gen_mulu2_i32(t2, t3, t2, t3);
>>> +            if (rd)
>>> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
>>> +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>>> +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
>>> +            tcg_temp_free_i32(t2);
>>> +            tcg_temp_free_i32(t3);
>>> +        }
>>> +        break;
>>> +    default:
>>> +        MIPS_INVAL("mul R5900");
>>> +        generate_exception_end(ctx, EXCP_RI);
>>> +        goto out;
>>> +    }
>>> +
>>> + out:
>>> +    tcg_temp_free(t0);
>>> +    tcg_temp_free(t1);
>>> +}
>>
>> Why not simply modify gen_muldiv?
> 
> Testing my own patch eh...
> 
> I missed the check_dsp() call:
> 
>     if (acc != 0 && unlikely(!(ctx->insn_flags & INSN_R5900))) {
>         check_dsp(ctx);
>     }
> 
> Anyway if we plan to also add MADD/MADDU, using another function might
> be cleaner.
> 
>>
>> -- >8 --
>> @@ -3630,29 +3630,35 @@ static void gen_muldiv(DisasContext *ctx,
>> uint32_t opc,
>>          break;
>>      case OPC_MULT:
>>          {
>>              TCGv_i32 t2 = tcg_temp_new_i32();
>>              TCGv_i32 t3 = tcg_temp_new_i32();
>>              tcg_gen_trunc_tl_i32(t2, t0);
>>              tcg_gen_trunc_tl_i32(t3, t1);
>>              tcg_gen_muls2_i32(t2, t3, t2, t3);
>> +            if (ctx->insn_flags & INSN_R5900 && rd) {
>> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
>> +            }
>>              tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>>              tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
>>              tcg_temp_free_i32(t2);
>>              tcg_temp_free_i32(t3);
>>          }
>>          break;
>>      case OPC_MULTU:
>>          {
>>              TCGv_i32 t2 = tcg_temp_new_i32();
>>              TCGv_i32 t3 = tcg_temp_new_i32();
>>              tcg_gen_trunc_tl_i32(t2, t0);
>>              tcg_gen_trunc_tl_i32(t3, t1);
>>              tcg_gen_mulu2_i32(t2, t3, t2, t3);
>> +            if (ctx->insn_flags & INSN_R5900 && rd) {
>> +                tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>> +            }
>>              tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
>>              tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
>>              tcg_temp_free_i32(t2);
>>              tcg_temp_free_i32(t3);
>>          }
>>          break;
>> ---
>>
>>> +
>>>  static void gen_mul_vr54xx (DisasContext *ctx, uint32_t opc,
>>>                              int rd, int rs, int rt)
>>>  {
>>> @@ -22378,6 +22429,8 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
>>>              check_insn(ctx, INSN_VR54XX);
>>>              op1 = MASK_MUL_VR54XX(ctx->opcode);
>>>              gen_mul_vr54xx(ctx, op1, rd, rs, rt);
>>> +        } else if (ctx->insn_flags & INSN_R5900) {
>>> +            gen_mul_r5900(ctx, op1, 0, rd, rs, rt);

And I now notice you take all the $rd bits, so my patch proposal should
also require:

            } else if (ctx->insn_flags & INSN_R5900) {
                gen_muldiv(ctx, op1, 0, rd, rs, rt);

Which a this point makes it pointless.

I'll restart reviewing your patch :)

>>>          } else {
>>>              gen_muldiv(ctx, op1, rd & 3, rs, rt);
>>>          }
>>>
Philippe Mathieu-Daudé Sept. 26, 2018, 10:25 p.m. UTC | #4 
On 9/15/18 11:25 AM, Fredrik Noring wrote:

Can you copy/paste some info regarding those instructions from the ISA
here, to note how they differ? ...

> Signed-off-by: Fredrik Noring <noring@nocrew.org>
> ---
>  target/mips/translate.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index ab16cdb911..fb571e278e 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -3768,6 +3768,57 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
>      tcg_temp_free(t1);
>  }
>  

... Another option is to describe in a big comment here.

> +static void gen_mul_r5900(DisasContext *ctx, uint32_t opc,
> +                          int acc, int rd, int rs, int rt)

Since we have acc = 0 we can directly use cpu_LO[0] and cpu_HI[0],
removing needs for an 'acc' argument.

> +{
> +    TCGv t0 = tcg_temp_new();
> +    TCGv t1 = tcg_temp_new();
> +
> +    gen_load_gpr(t0, rs);
> +    gen_load_gpr(t1, rt);
> +
> +    switch (opc) {
> +    case OPC_MULT:
> +        {
> +            TCGv_i32 t2 = tcg_temp_new_i32();
> +            TCGv_i32 t3 = tcg_temp_new_i32();
> +            tcg_gen_trunc_tl_i32(t2, t0);
> +            tcg_gen_trunc_tl_i32(t3, t1);
> +            tcg_gen_muls2_i32(t2, t3, t2, t3);
> +            if (rd)

Check QEMU CODING_STYLE "Block structure":

  Every indented statement is braced; even if the block contains
  just one statement.  The opening brace is on the line that
  contains the control flow statement that introduces the new block;
  the closing brace is on the same line as the else keyword, or on
  a line by itself if there is no else keyword.

QEMU scripts/checkpatch.pl reports such mistakes.

> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);

I'd use:

                   gen_move_low32(cpu_gpr[rd], t2);

> +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);

So:

               tcg_gen_ext_i32_tl(cpu_LO[0], t2);

> +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);

And:

               tcg_gen_ext_i32_tl(cpu_HI[0], t3);

> +            tcg_temp_free_i32(t2);
> +            tcg_temp_free_i32(t3);
> +        }
> +        break;
> +    case OPC_MULTU:
> +        {
> +            TCGv_i32 t2 = tcg_temp_new_i32();
> +            TCGv_i32 t3 = tcg_temp_new_i32();
> +            tcg_gen_trunc_tl_i32(t2, t0);
> +            tcg_gen_trunc_tl_i32(t3, t1);
> +            tcg_gen_mulu2_i32(t2, t3, t2, t3);
> +            if (rd)
> +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);

Same comments from MULT apply here.

> +            tcg_temp_free_i32(t2);
> +            tcg_temp_free_i32(t3);
> +        }
> +        break;
> +    default:
> +        MIPS_INVAL("mul R5900");
> +        generate_exception_end(ctx, EXCP_RI);
> +        goto out;
> +    }
> +
> + out:
> +    tcg_temp_free(t0);
> +    tcg_temp_free(t1);
> +}
> +
>  static void gen_mul_vr54xx (DisasContext *ctx, uint32_t opc,
>                              int rd, int rs, int rt)
>  {
> @@ -22378,6 +22429,8 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
>              check_insn(ctx, INSN_VR54XX);
>              op1 = MASK_MUL_VR54XX(ctx->opcode);
>              gen_mul_vr54xx(ctx, op1, rd, rs, rt);
> +        } else if (ctx->insn_flags & INSN_R5900) {
> +            gen_mul_r5900(ctx, op1, 0, rd, rs, rt);

Removing 'acc' arg:

               gen_mul_r5900(ctx, op1, rd, rs, rt);

Note, these instructions are also valid on the R3900 (which also has
MADD/MADDU).

Would gen_mul_toshiba() be a better common name? I don't like it but
can't think of another.

>          } else {
>              gen_muldiv(ctx, op1, rd & 3, rs, rt);
>          }
> 
Regards,

Phil.
Maciej W. Rozycki Sept. 26, 2018, 10:43 p.m. UTC | #5 
On Thu, 27 Sep 2018, Philippe Mathieu-Daudé wrote:

> > +static void gen_mul_r5900(DisasContext *ctx, uint32_t opc,
> > +                          int acc, int rd, int rs, int rt)
> 
> Since we have acc = 0 we can directly use cpu_LO[0] and cpu_HI[0],
> removing needs for an 'acc' argument.

 Corresponding MULT1 and MULTU1 instructions use `acc = 1' though, as do 
similar variations of all the other MDU instructions.  Maybe the addition 
of the required `acc' argument could be staged for a separate patch with 
all the pipeline 1 MDU instructions though.

  Maciej
Fredrik Noring Sept. 28, 2018, 3:15 p.m. UTC | #6 
Hi Philippe,

> Can you copy/paste some info regarding those instructions from the ISA
> here, to note how they differ? ...

Yes. Other corresponding functions typically do not seem to have such ISA
notes, but I can certainly write one for it.

> Since we have acc = 0 we can directly use cpu_LO[0] and cpu_HI[0],
> removing needs for an 'acc' argument.

We could, but as Maciej replied there are pipeline 1 versions of these
instructions which would have acc = 1. I retained the acc parameter mainly
to avoid the error of forgetting to replace 0 with acc when introducing
them later on.

> > +{
> > +    TCGv t0 = tcg_temp_new();
> > +    TCGv t1 = tcg_temp_new();
> > +
> > +    gen_load_gpr(t0, rs);
> > +    gen_load_gpr(t1, rt);
> > +
> > +    switch (opc) {
> > +    case OPC_MULT:
> > +        {
> > +            TCGv_i32 t2 = tcg_temp_new_i32();
> > +            TCGv_i32 t3 = tcg_temp_new_i32();
> > +            tcg_gen_trunc_tl_i32(t2, t0);
> > +            tcg_gen_trunc_tl_i32(t3, t1);
> > +            tcg_gen_muls2_i32(t2, t3, t2, t3);
> > +            if (rd)
> 
> Check QEMU CODING_STYLE "Block structure":
> 
>   Every indented statement is braced; even if the block contains
>   just one statement.  The opening brace is on the line that
>   contains the control flow statement that introduces the new block;
>   the closing brace is on the same line as the else keyword, or on
>   a line by itself if there is no else keyword.
> 
> QEMU scripts/checkpatch.pl reports such mistakes.

Done.

> > +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> 
> I'd use:
> 
>                    gen_move_low32(cpu_gpr[rd], t2);

Are you sure this is correct? The GPR rd must be sign-extended.

> > +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> 
> So:
> 
>                tcg_gen_ext_i32_tl(cpu_LO[0], t2);
> 
> > +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
> 
> And:
> 
>                tcg_gen_ext_i32_tl(cpu_HI[0], t3);

Done.

> > +            tcg_temp_free_i32(t2);
> > +            tcg_temp_free_i32(t3);
> > +        }
> > +        break;
> > +    case OPC_MULTU:
> > +        {
> > +            TCGv_i32 t2 = tcg_temp_new_i32();
> > +            TCGv_i32 t3 = tcg_temp_new_i32();
> > +            tcg_gen_trunc_tl_i32(t2, t0);
> > +            tcg_gen_trunc_tl_i32(t3, t1);
> > +            tcg_gen_mulu2_i32(t2, t3, t2, t3);
> > +            if (rd)
> > +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> > +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> > +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
> 
> Same comments from MULT apply here.

Done.

> > +            tcg_temp_free_i32(t2);
> > +            tcg_temp_free_i32(t3);
> > +        }
> > +        break;
> > +    default:
> > +        MIPS_INVAL("mul R5900");
> > +        generate_exception_end(ctx, EXCP_RI);
> > +        goto out;
> > +    }
> > +
> > + out:
> > +    tcg_temp_free(t0);
> > +    tcg_temp_free(t1);
> > +}
> > +
> >  static void gen_mul_vr54xx (DisasContext *ctx, uint32_t opc,
> >                              int rd, int rs, int rt)
> >  {
> > @@ -22378,6 +22429,8 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
> >              check_insn(ctx, INSN_VR54XX);
> >              op1 = MASK_MUL_VR54XX(ctx->opcode);
> >              gen_mul_vr54xx(ctx, op1, rd, rs, rt);
> > +        } else if (ctx->insn_flags & INSN_R5900) {
> > +            gen_mul_r5900(ctx, op1, 0, rd, rs, rt);
> 
> Removing 'acc' arg:
> 
>                gen_mul_r5900(ctx, op1, rd, rs, rt);

Done.

> Note, these instructions are also valid on the R3900 (which also has
> MADD/MADDU).
> 
> Would gen_mul_toshiba() be a better common name? I don't like it but
> can't think of another.

I propose gen_mul_3op, since its distinctive feature is the three operands.

Fredrik
Philippe Mathieu-Daudé Sept. 28, 2018, 3:34 p.m. UTC | #7 
On Fri, Sep 28, 2018 at 5:16 PM Fredrik Noring <noring@nocrew.org> wrote:
>
> Hi Philippe,
>
> > Can you copy/paste some info regarding those instructions from the ISA
> > here, to note how they differ? ...
>
> Yes. Other corresponding functions typically do not seem to have such ISA
> notes, but I can certainly write one for it.

My guess is they don't have because they implement the generic ISA
which is public.

It is rather hard to find public manuals for the Toshiba/Sony ISA.

> > Since we have acc = 0 we can directly use cpu_LO[0] and cpu_HI[0],
> > removing needs for an 'acc' argument.
>
> We could, but as Maciej replied there are pipeline 1 versions of these
> instructions which would have acc = 1. I retained the acc parameter mainly
> to avoid the error of forgetting to replace 0 with acc when introducing
> them later on.

Yes, I was not aware of those instructions, so better keep the 'acc' argument.

> > > +{
> > > +    TCGv t0 = tcg_temp_new();
> > > +    TCGv t1 = tcg_temp_new();
> > > +
> > > +    gen_load_gpr(t0, rs);
> > > +    gen_load_gpr(t1, rt);
> > > +
> > > +    switch (opc) {
> > > +    case OPC_MULT:
> > > +        {
> > > +            TCGv_i32 t2 = tcg_temp_new_i32();
> > > +            TCGv_i32 t3 = tcg_temp_new_i32();
> > > +            tcg_gen_trunc_tl_i32(t2, t0);
> > > +            tcg_gen_trunc_tl_i32(t3, t1);
> > > +            tcg_gen_muls2_i32(t2, t3, t2, t3);
> > > +            if (rd)
> >
> > Check QEMU CODING_STYLE "Block structure":
> >
> >   Every indented statement is braced; even if the block contains
> >   just one statement.  The opening brace is on the line that
> >   contains the control flow statement that introduces the new block;
> >   the closing brace is on the same line as the else keyword, or on
> >   a line by itself if there is no else keyword.
> >
> > QEMU scripts/checkpatch.pl reports such mistakes.
>
> Done.
>
> > > +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> >
> > I'd use:
> >
> >                    gen_move_low32(cpu_gpr[rd], t2);
>
> Are you sure this is correct? The GPR rd must be sign-extended.

Richard can you verify please?

> > > +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> >
> > So:
> >
> >                tcg_gen_ext_i32_tl(cpu_LO[0], t2);
> >
> > > +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
> >
> > And:
> >
> >                tcg_gen_ext_i32_tl(cpu_HI[0], t3);
>
> Done.
>
> > > +            tcg_temp_free_i32(t2);
> > > +            tcg_temp_free_i32(t3);
> > > +        }
> > > +        break;
> > > +    case OPC_MULTU:
> > > +        {
> > > +            TCGv_i32 t2 = tcg_temp_new_i32();
> > > +            TCGv_i32 t3 = tcg_temp_new_i32();
> > > +            tcg_gen_trunc_tl_i32(t2, t0);
> > > +            tcg_gen_trunc_tl_i32(t3, t1);
> > > +            tcg_gen_mulu2_i32(t2, t3, t2, t3);
> > > +            if (rd)
> > > +                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
> > > +            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
> > > +            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
> >
> > Same comments from MULT apply here.
>
> Done.
>
> > > +            tcg_temp_free_i32(t2);
> > > +            tcg_temp_free_i32(t3);
> > > +        }
> > > +        break;
> > > +    default:
> > > +        MIPS_INVAL("mul R5900");
> > > +        generate_exception_end(ctx, EXCP_RI);
> > > +        goto out;
> > > +    }
> > > +
> > > + out:
> > > +    tcg_temp_free(t0);
> > > +    tcg_temp_free(t1);
> > > +}
> > > +
> > >  static void gen_mul_vr54xx (DisasContext *ctx, uint32_t opc,
> > >                              int rd, int rs, int rt)
> > >  {
> > > @@ -22378,6 +22429,8 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
> > >              check_insn(ctx, INSN_VR54XX);
> > >              op1 = MASK_MUL_VR54XX(ctx->opcode);
> > >              gen_mul_vr54xx(ctx, op1, rd, rs, rt);
> > > +        } else if (ctx->insn_flags & INSN_R5900) {
> > > +            gen_mul_r5900(ctx, op1, 0, rd, rs, rt);
> >
> > Removing 'acc' arg:
> >
> >                gen_mul_r5900(ctx, op1, rd, rs, rt);
>
> Done.
>
> > Note, these instructions are also valid on the R3900 (which also has
> > MADD/MADDU).
> >
> > Would gen_mul_toshiba() be a better common name? I don't like it but
> > can't think of another.
>
> I propose gen_mul_3op, since its distinctive feature is the three operands.

Fine by me.
Maciej W. Rozycki Sept. 28, 2018, 3:59 p.m. UTC | #8 
On Fri, 28 Sep 2018, Philippe Mathieu-Daudé wrote:

> > > Note, these instructions are also valid on the R3900 (which also has
> > > MADD/MADDU).
> > >
> > > Would gen_mul_toshiba() be a better common name? I don't like it but
> > > can't think of another.
> >
> > I propose gen_mul_3op, since its distinctive feature is the three operands.
> 
> Fine by me.

 Bikeshedding, but...  I haven't looked at how we implement it, however 
generic MIPS architecture also has a 3-operand MUL instruction defined, 
reusing, for R1-R5, the encoding used earlier on by IDT R4650 and NEC 
Vr5500, and using a different one for R6.  So I'd rather we avoided 
confusion here.

 Perhaps `gen_mul_ee' then for Emotion Engine? -- EE is what libopcodes 
uses for R5900 instructions where brevity matters.  Or `gen_mul_4op', 
because we have 4 operands really, with the HI/LO accumulator being an 
implicit one.

 What's wrong with `gen_mul_r5900' anyway?

  Maciej
Philippe Mathieu-Daudé Sept. 28, 2018, 4:37 p.m. UTC | #9 
On Fri, Sep 28, 2018 at 5:59 PM Maciej W. Rozycki <macro@linux-mips.org> wrote:
>
> On Fri, 28 Sep 2018, Philippe Mathieu-Daudé wrote:
>
> > > > Note, these instructions are also valid on the R3900 (which also has
> > > > MADD/MADDU).
> > > >
> > > > Would gen_mul_toshiba() be a better common name? I don't like it but
> > > > can't think of another.
> > >
> > > I propose gen_mul_3op, since its distinctive feature is the three operands.
> >
> > Fine by me.
>
>  Bikeshedding, but...  I haven't looked at how we implement it, however
> generic MIPS architecture also has a 3-operand MUL instruction defined,
> reusing, for R1-R5, the encoding used earlier on by IDT R4650 and NEC
> Vr5500, and using a different one for R6.  So I'd rather we avoided
> confusion here.
>
>  Perhaps `gen_mul_ee' then for Emotion Engine? -- EE is what libopcodes
> uses for R5900 instructions where brevity matters.  Or `gen_mul_4op',
> because we have 4 operands really, with the HI/LO accumulator being an
> implicit one.
>
>  What's wrong with `gen_mul_r5900' anyway?

I plan to use this function (adding MADD/MADDU) for R3900 based cores
(which don't seemt related to Emotion Engine).
Maciej W. Rozycki Sept. 28, 2018, 5:15 p.m. UTC | #10 
On Fri, 28 Sep 2018, Philippe Mathieu-Daudé wrote:

> >  What's wrong with `gen_mul_r5900' anyway?
> 
> I plan to use this function (adding MADD/MADDU) for R3900 based cores
> (which don't seemt related to Emotion Engine).

 Fair enough.  I reached for documentation and these instructions seem to 
be shared by all members of Toshiba's TX System RISC family, i.e. TX19, 
TX39, etc.  So I think `gen_mul_toshiba' or `gen_mul_tx' or `gen_mul_txxx' 
will do.

  Maciej
diff mbox series

Patch

diff --git a/target/mips/translate.c b/target/mips/translate.c
index ab16cdb911..fb571e278e 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -3768,6 +3768,57 @@  static void gen_muldiv(DisasContext *ctx, uint32_t opc,
     tcg_temp_free(t1);
 }
 
+static void gen_mul_r5900(DisasContext *ctx, uint32_t opc,
+                          int acc, int rd, int rs, int rt)
+{
+    TCGv t0 = tcg_temp_new();
+    TCGv t1 = tcg_temp_new();
+
+    gen_load_gpr(t0, rs);
+    gen_load_gpr(t1, rt);
+
+    switch (opc) {
+    case OPC_MULT:
+        {
+            TCGv_i32 t2 = tcg_temp_new_i32();
+            TCGv_i32 t3 = tcg_temp_new_i32();
+            tcg_gen_trunc_tl_i32(t2, t0);
+            tcg_gen_trunc_tl_i32(t3, t1);
+            tcg_gen_muls2_i32(t2, t3, t2, t3);
+            if (rd)
+                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
+            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
+            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
+            tcg_temp_free_i32(t2);
+            tcg_temp_free_i32(t3);
+        }
+        break;
+    case OPC_MULTU:
+        {
+            TCGv_i32 t2 = tcg_temp_new_i32();
+            TCGv_i32 t3 = tcg_temp_new_i32();
+            tcg_gen_trunc_tl_i32(t2, t0);
+            tcg_gen_trunc_tl_i32(t3, t1);
+            tcg_gen_mulu2_i32(t2, t3, t2, t3);
+            if (rd)
+                tcg_gen_ext_i32_tl(cpu_gpr[rd], t2);
+            tcg_gen_ext_i32_tl(cpu_LO[acc], t2);
+            tcg_gen_ext_i32_tl(cpu_HI[acc], t3);
+            tcg_temp_free_i32(t2);
+            tcg_temp_free_i32(t3);
+        }
+        break;
+    default:
+        MIPS_INVAL("mul R5900");
+        generate_exception_end(ctx, EXCP_RI);
+        goto out;
+    }
+
+ out:
+    tcg_temp_free(t0);
+    tcg_temp_free(t1);
+}
+
 static void gen_mul_vr54xx (DisasContext *ctx, uint32_t opc,
                             int rd, int rs, int rt)
 {
@@ -22378,6 +22429,8 @@  static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
             check_insn(ctx, INSN_VR54XX);
             op1 = MASK_MUL_VR54XX(ctx->opcode);
             gen_mul_vr54xx(ctx, op1, rd, rs, rt);
+        } else if (ctx->insn_flags & INSN_R5900) {
+            gen_mul_r5900(ctx, op1, 0, rd, rs, rt);
         } else {
             gen_muldiv(ctx, op1, rd & 3, rs, rt);
         }