Re: [PATCH 07/20] target/loongarch: Add fixed point arithmetic instruction translation

[Philippe Mathieu-Daudé]

On 7/2/21 10:15 AM, Song Gao wrote:
> On 07/02/2021 04:31 AM, Philippe Mathieu-Daudé wrote:
>> On 6/28/21 2:04 PM, Song Gao wrote:
>>> This patch implement fixed point arithemtic instruction translation.
>>>
>>> This includes:
>>> - ADD.{W/D}, SUB.{W/D}
>>> - ADDI.{W/D}, ADDU16ID
>>> - ALSL.{W[U]/D}
>>> - LU12I.W, LU32I.D LU52I.D
>>> - SLT[U], SLT[U]I
>>> - PCADDI, PCADDU12I, PCADDU18I, PCALAU12I
>>> - AND, OR, NOR, XOR, ANDN, ORN
>>> - MUL.{W/D}, MULH.{W[U]/D[U]}
>>> - MULW.D.W[U]
>>> - DIV.{W[U]/D[U]}, MOD.{W[U]/D[U]}
>>> - ANDI, ORI, XORI
>>>
>>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>>> ---
>>>  target/loongarch/insns.decode |  89 ++++++++
>>>  target/loongarch/instmap.h    |  53 +++++
>>>  target/loongarch/trans.inc.c  | 367 +++++++++++++++++++++++++++++++++
>>>  target/loongarch/translate.c  | 458 
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 967 insertions(+)
>>>  create mode 100644 target/loongarch/insns.decode
>>>  create mode 100644 target/loongarch/instmap.h
>>>  create mode 100644 target/loongarch/trans.inc.c

>> It seems you are missing what decodetree is for... You should inline
>> each opcode code from gen_loongarch_muldiv in the opcode handler.
>>
>> Don't take MIPS as an example =)
>>
> Hi, Philippe,
> 
> I‘m not sure I understand right.  Here is an example of my modification
> 
>     static bool trans_xxx(DisasContext *ctx, arg_mul_w *a)
>     {
>         gen_loongarch_muldiv(ctx, a->rd, a->rj, a->rk);
>         return true;
>     }
>     ...
> 
>     static void gen_loongarch_muldiv(DisasContext *ctx, int rd,
>                                      int rj, int rk)
>     {
>         TCGv t0, t1;
> 
>         if (rd == 0) {
>             /* Treat as NOP. */
>             return;
>         }
> 
>         t0 = tcg_temp_new();
>         t1 = tcg_temp_new();
> 
>         gen_load_gpr(t0, rj);
>         gen_load_gpr(t1, rk);
> 
>         switch (ctx->opcode) {
>         case  xxx_opcode:
>              /* translate  xxx  */
>         ...
> 
>     }
> 
> Is that right？

No. With decode"tree" you only have to implement the
decode"leaves". No need to pass 'uin32_t opcode' and
use 'switch (ctx->opcode) ...'. Example for LA_OPC_MUL_D:

static bool trans_mul_d(DisasContext *ctx, int rd, int rj, int rk)
{
    TCGv t0, t1;

    check_loongarch_64(ctx);

    if (a->rd == 0) {
        /* Treat as NOP. */
        return true;
    }

    t0 = tcg_temp_new();
    t1 = tcg_temp_new();

    gen_load_gpr(t0, a->rj);
    gen_load_gpr(t1, a->rk);

    tcg_gen_mul_i64(cpu_gpr[a->rd], t0, t1);

    tcg_temp_free(t0);
    tcg_temp_free(t1);

    return true;
}