Re: [PATCH v10 16/26] target/loongarch: Add disassembler

[Richard Henderson <richard.henderson@linaro.org>]

On 11/12/21 7:53 AM, Song Gao wrote:
> +const char * const fccregnames[8] = {
> +  "$fcc0", "$fcc1", "$fcc2", "$fcc3", "$fcc4", "$fcc5", "$fcc6", "$fcc7",
> +};

static.

> +static void output_fcsrdrj(DisasContext *ctx, arg_fmt_fcsrdrj *a,
> +                           const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, %s", regnames[a->fcsrd], regnames[a->rj]);
> +}
> +
> +static void output_rdfcsrs(DisasContext *ctx, arg_fmt_rdfcsrs *a,
> +                           const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, %s", regnames[a->rd], regnames[a->fcsrs]);
> +}

Wrong names for fcsr[ds].  Probably easiest to just use "fcsr%d" rather than having an 
array of strings.

> +static void output_rdrjsi12(DisasContext *ctx, arg_fmt_rdrjsi12 *a,
> +                            const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, %s, 0x%x",
> +           regnames[a->rd], regnames[a->rj], (a->si12) & 0xfff);
> +}

Surely printing the signed value is more useful.

> +static void output_rdrjsi16(DisasContext *ctx, arg_fmt_rdrjsi16 *a,
> +                            const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, %s, 0x%x",
> +           regnames[a->rd], regnames[a->rj], (a->si16) & 0xffff);
> +}
> +
> +static void output_rdsi20(DisasContext *ctx, arg_fmt_rdsi20 *a,
> +                          const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, 0x%x", regnames[a->rd], (a->si20) & 0xfffff);
> +}
> +
> +static void output_rdrjsi14(DisasContext *ctx, arg_fmt_rdrjsi14 *a,
> +                            const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, %s, 0x%x",
> +           regnames[a->rd], regnames[a->rj],  (a->si14) & 0x3fff);
> +}
> +
> +static void output_hintrjsi12(DisasContext *ctx, arg_fmt_hintrjsi12 *a,
> +                              const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "0x%x, %s, 0x%x",
> +           a->hint, regnames[a->rj], (a->si12) & 0xfff);
> +}
> +
> +static void output_fdrjsi12(DisasContext *ctx, arg_fmt_fdrjsi12 *a,
> +                            const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, %s, 0x%x",
> +           fregnames[a->fd], regnames[a->rj], (a->si12) & 0xfff);
> +}

Likewise.

> +static void output_rjoffs21(DisasContext *ctx, arg_fmt_rjoffs21 *a,
> +                            const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, 0x%x", regnames[a->rj], (a->offs21) & 0x1fffff);
> +}
> +
> +static void output_cjoffs21(DisasContext *ctx, arg_fmt_cjoffs21 *a,
> +                            const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, 0x%x",
> +           fccregnames[a->cj], (a->offs21) & 0x1fffff);
> +}
> +
> +static void output_rdrjoffs16(DisasContext *ctx, arg_fmt_rdrjoffs16 *a,
> +                              const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, %s, 0x%x",
> +           regnames[a->rd], regnames[a->rj], (a->offs16) & 0xffff);
> +}
> +
> +static void output_offs(DisasContext *ctx, arg_fmt_offs *a,
> +                        const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "0x%x", (a->offs) & 0x3ffffff);
> +}

These are signed, but they're also pc-relative.  It's probably most helpful to have stored 
the address into ctx and compute the final address.

> +static void output_rdfj(DisasContext *ctx, arg_fmt_rdfj *a,
> +                        const char *mnemonic)
> +{
> +    output(ctx, mnemonic, "%s, %s", regnames[a->rd], regnames[a->fj]);
> +}

Wrong name for fj.

> +#define output_fcmp(C, PREFIX, SUBFFIX)                                     \

SUFFIX

> +static void output_cdfjfkfcond(DisasContext *ctx, arg_fmt_cdfjfkfcond * a,
> +                               const char *suffix)
> +{
> +    switch (a->fcond) {
> +    case 0x0:
> +        output_fcmp(ctx, "fcmp_caf_", suffix);
> +        break;
> +    case 0x1:
> +        output_fcmp(ctx, "fcmp_saf_", suffix);
> +        break;
> +    case 0x2:
> +        output_fcmp(ctx, "fcmp_clt_", suffix);
> +        break;
> +    case 0x3:
> +        output_fcmp(ctx, "fcmp_slt_", suffix);
> +        break;
> +    case 0x4:
> +        output_fcmp(ctx, "fcmp_ceq_", suffix);
> +        break;
> +    case 0x5:
> +        output_fcmp(ctx, "fcmp_seq_", suffix);
> +        break;
> +    case 0x6:
> +        output_fcmp(ctx, "fcmp_cle_", suffix);
> +        break;
> +    case 0x7:
> +        output_fcmp(ctx, "fcmp_sle_", suffix);
> +        break;
> +    case 0x8:
> +        output_fcmp(ctx, "fcmp_cun_", suffix);
> +        break;
> +    case 0x9:
> +        output_fcmp(ctx, "fcmp_sun_", suffix);
> +        break;
> +    case 0xA:
> +        output_fcmp(ctx, "fcmp_cult_", suffix);
> +        break;
> +    case 0xB:
> +        output_fcmp(ctx, "fcmp_sult_", suffix);
> +        break;
> +    case 0xC:
> +        output_fcmp(ctx, "fcmp_cueq_", suffix);
> +        break;
> +    case 0xD:
> +        output_fcmp(ctx, "fcmp_sueq_", suffix);
> +        break;
> +    case 0xE:
> +        output_fcmp(ctx, "fcmp_cule_", suffix);
> +        break;
> +    case 0xF:
> +        output_fcmp(ctx, "fcmp_sule_", suffix);
> +        break;
> +    case 0x10:
> +        output_fcmp(ctx, "fcmp_cne_", suffix);
> +        break;
> +    case 0x11:
> +        output_fcmp(ctx, "fcmp_sne_", suffix);
> +        break;
> +    case 0x14:
> +        output_fcmp(ctx, "fcmp_cor_", suffix);
> +        break;
> +    case 0x15:
> +        output_fcmp(ctx, "fcmp_sor_", suffix);
> +        break;
> +    case 0x18:
> +        output_fcmp(ctx, "fcmp_cune_", suffix);
> +        break;
> +    case 0x19:
> +        output_fcmp(ctx, "fcmp_sune_", suffix);
> +        break;
> +    default:
> +        break;

Here you're going to print nothing at all, which is wrong.

> +    }
> +}
> +
> +#define FCMP_INSN(insn, suffix, type)                           \
> +static bool trans_##insn(DisasContext *ctx, arg_fmt_##type * a) \
> +{                                                               \
> +    output_##type(ctx, a, #suffix);                             \
> +    return true;                                                \
> +}

I think you should drop "insn" and "type" from this define and use output_cdfjfkfcond 
directly.

I think that FCMP_INSN should return output_cdfjfkfcond, which should return false for the 
default case, so that decodetree returns false, so that we print "invalid".


r~