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~