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

[gaosong]

Hi Richard,

On 2021/11/12 下午3:39, Richard Henderson wrote:
> On 11/12/21 7:53 AM, Song Gao wrote:
> > +const char * const fccregnames[8] = {
> > +  "$fcc0", "$fcc1", "$fcc2", "$fcc3", "$fcc4", "$fcc5", "$fcc6", 
> > "$fcc7",
> > +};
>
> static.
OK.
> > +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.
OK.
> > +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.
This's a good idea.
> > +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
>
> > +        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:
> >
> > +        break;
>
> Here you're going to print nothing at all, which is wrong.
OK.
> > +    }
> > +}
> > +
> > +#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".
Got it,  I'll correct it on V11.
> r~