[Qemu-devel] [RFC PATCH 00/11] decodetree: Add tokens to ease checking ISA flags

[Qemu-devel] [RFC PATCH 00/11] decodetree: Add tokens to ease checking ISA flags

[Philippe Mathieu-Daudé]

Hi Richard,

I have been wondering how we can simplify when dealing with multiple ISAs.
I used the MIPS arch because it aims to be simple, but handling the multiple
ISAs/ASEs as once is a mess, with the particular case of the MIPS32R6.

First I wanted to split the translate.c in various ISA/ASE-related files,
but this there are too many inlined func involved, I found it handy to use
the ?cond token, so we can link all translate functions without worrying
about #ifdef'ry.
The translating functions are now smaller/easier to read.

Then I wanted to add stricter ISA check, to not deal with multiple
specifications included more than once, and ease overlapping patterns.

I'm not super happy with this series (in particular the token added
are MIPS oriented, you can not use spaces in the condition), but I'm
interested by what you think :)

Rebasing decodetree specs is painful, so better figure what's the best
now before continuing.

At some point I'd like to get to the one ISA/ASE per file, so we can share
compilation units between targets (via $common-obj) and also be able to
disable completely some ISAs, for whatever reason (downstream/obsolete/...).

Regards,

Phil.

Philippe Mathieu-Daudé (11):
  MAINTAINERS: Add scripts/decodetree.py to the TCG section
  decodetree: Add multiple include guard
  target/mips: Move the !ISA_MIPS32R6 check out of
    decode_opc_special2_legacy
  target/mips: Avoid access to CPUMIPSState from decode* functions
  decodetree: Force Python to print unsigned values
  scripts/decodetree: Allow empty specifications
  scripts/decodetree: Add add_func_check()
  target/mips: Add a decodetree stub
  target/mips: Port SYNCI to decodetree
  scripts/decodetree: Add add_cond_check()
  target/mips: Port MIPS64 DCL[Z/O] to decodetree

 MAINTAINERS                 |  1 +
 scripts/decodetree.py       | 62 ++++++++++++++++++++++++++++++-------
 target/mips/Makefile.objs   |  8 +++++
 target/mips/insns.decode    | 22 +++++++++++++
 target/mips/translate.c     | 20 ++++++------
 target/mips/translate.inc.c | 32 +++++++++++++++++++
 6 files changed, 123 insertions(+), 22 deletions(-)
 create mode 100644 target/mips/insns.decode
 create mode 100644 target/mips/translate.inc.c

-- 
2.17.2

[Qemu-devel] [RFC PATCH 01/11] MAINTAINERS: Add scripts/decodetree.py to the TCG section

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 126fe0be7e..45e4bfcd87 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -112,6 +112,7 @@ S: Maintained
 F: cpus.c
 F: exec.c
 F: accel/tcg/
+F: scripts/decodetree.py
 F: include/exec/cpu*.h
 F: include/exec/exec-all.h
 F: include/exec/helper*.h
-- 
2.17.2

[Qemu-devel] [RFC PATCH 02/11] decodetree: Add multiple include guard

[Philippe Mathieu-Daudé]

It is necessary when splitting an ISA, or when using multiple ISAs.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
TODO: explain why, use case
TODO: escape full path?
---
 scripts/decodetree.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scripts/decodetree.py b/scripts/decodetree.py
index 0bc73b5990..5dea15e7a5 100755
--- a/scripts/decodetree.py
+++ b/scripts/decodetree.py
@@ -1030,7 +1030,11 @@ def main():
     else:
         output_fd = sys.stdout
 
+    hdr_guard = filename.split(os.path.sep)[-1].upper().replace('.', '_') + "_H"
+
     output_autogen()
+    output('#ifndef ' + hdr_guard + '\n')
+    output('#define ' + hdr_guard + '\n')
     for n in sorted(arguments.keys()):
         f = arguments[n]
         f.output_def()
@@ -1066,6 +1070,7 @@ def main():
     t.output_code(4, False, 0, 0)
 
     output('}\n')
+    output('#endif /* ' + hdr_guard + ' */\n')
 
     if output_file:
         output_fd.close()
-- 
2.17.2

[Qemu-devel] [RFC PATCH 03/11] target/mips: Move the !ISA_MIPS32R6 check out of decode_opc_special2_legacy

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/translate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 60320cbe69..f5e8d0b4d2 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -25649,8 +25649,6 @@ static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx)
     int rs, rt, rd;
     uint32_t op1;
 
-    check_insn_opc_removed(ctx, ISA_MIPS32R6);
-
     rs = (ctx->opcode >> 21) & 0x1f;
     rt = (ctx->opcode >> 16) & 0x1f;
     rd = (ctx->opcode >> 11) & 0x1f;
@@ -27890,6 +27888,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
         } else if (ctx->insn_flags & ASE_MXU) {
             decode_opc_mxu(env, ctx);
         } else {
+            check_insn_opc_removed(ctx, ISA_MIPS32R6);
             decode_opc_special2_legacy(env, ctx);
         }
         break;
-- 
2.17.2

[Qemu-devel] [RFC PATCH 04/11] target/mips: Avoid access to CPUMIPSState from decode* functions

[Philippe Mathieu-Daudé]

The DisasContext is already initialized from the CPUMIPSState in
mips_tr_init_disas_context().

Suggested-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index f5e8d0b4d2..e726f3ec00 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -16534,7 +16534,7 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
             check_insn(ctx, ASE_MIPS3D);
             /* Fall through */
         do_cp1branch:
-            if (env->CP0_Config1 & (1 << CP0C1_FP)) {
+            if (ctx->CP0_Config1 & (1 << CP0C1_FP)) {
                 check_cp1_enabled(ctx);
                 gen_compute_branch1(ctx, mips32_op,
                                     (ctx->opcode >> 18) & 0x7, imm << 1);
@@ -23809,7 +23809,7 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
         break;
     case OPC_MOVCI:
         check_insn(ctx, ISA_MIPS4 | ISA_MIPS32);
-        if (env->CP0_Config1 & (1 << CP0C1_FP)) {
+        if (ctx->CP0_Config1 & (1 << CP0C1_FP)) {
             check_cp1_enabled(ctx);
             gen_movci(ctx, rd, rs, (ctx->opcode >> 18) & 0x7,
                       (ctx->opcode >> 16) & 1);
-- 
2.17.2

[Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Philippe Mathieu-Daudé]

Python internal representation is signed, so unsigned values
bigger than 31-bit are interpreted as signed (and printed with
a '-' signed).
Mask out to force unsigned values.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
TODO: display error encountered:

   case 0x-1:
       ....
---
 scripts/decodetree.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/decodetree.py b/scripts/decodetree.py
index 5dea15e7a5..08aa52d544 100755
--- a/scripts/decodetree.py
+++ b/scripts/decodetree.py
@@ -900,12 +900,12 @@ class Tree:
             def str_case(b):
                 return '0x{0:08x}'.format(b)
 
-        output(ind, 'switch (', str_switch(self.thismask), ') {\n')
+        output(ind, 'switch (', str_switch(self.thismask & insnmask), ') {\n')
         for b, s in sorted(self.subs):
             assert (self.thismask & ~s.fixedmask) == 0
             innermask = outermask | self.thismask
             innerbits = outerbits | b
-            output(ind, 'case ', str_case(b), ':\n')
+            output(ind, 'case ', str_case(b & insnmask), ':\n')
             output(ind, '    /* ',
                    str_match_bits(innerbits, innermask), ' */\n')
             s.output_code(i + 4, extracted, innerbits, innermask)
-- 
2.17.2

[Qemu-devel] [RFC PATCH 06/11] scripts/decodetree: Allow empty specifications

[Philippe Mathieu-Daudé]

Starting with empty specifications allow to write stubs/templates,
useful when testing/rebasing.

This fixes:

  decode.inc.c: In function ‘decode’:
  decode.inc.c:9:7: error: unused variable ‘u’ [-Werror=unused-variable]
     } u;
       ^

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 scripts/decodetree.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/scripts/decodetree.py b/scripts/decodetree.py
index 08aa52d544..41ed67132d 100755
--- a/scripts/decodetree.py
+++ b/scripts/decodetree.py
@@ -1060,12 +1060,13 @@ def main():
     output(decode_scope, 'bool ', decode_function,
            '(DisasContext *ctx, ', insntype, ' insn)\n{\n')
 
-    i4 = str_indent(4)
-    output(i4, 'union {\n')
-    for n in sorted(arguments.keys()):
-        f = arguments[n]
-        output(i4, i4, f.struct_name(), ' f_', f.name, ';\n')
-    output(i4, '} u;\n\n')
+    if arguments:
+        i4 = str_indent(4)
+        output(i4, 'union {\n')
+        for n in sorted(arguments.keys()):
+            f = arguments[n]
+            output(i4, i4, f.struct_name(), ' f_', f.name, ';\n')
+        output(i4, '} u;\n\n')
 
     t.output_code(4, False, 0, 0)
 
-- 
2.17.2

[Qemu-devel] [RFC PATCH 07/11] scripts/decodetree: Add add_func_check()

[Philippe Mathieu-Daudé]

The '>' token allow to call a check(arg) function.

This is useful to assert an instruction is supported by an ISA.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 scripts/decodetree.py | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/scripts/decodetree.py b/scripts/decodetree.py
index 41ed67132d..2450cc1a63 100755
--- a/scripts/decodetree.py
+++ b/scripts/decodetree.py
@@ -420,7 +420,7 @@ class Arguments:
 
 class General:
     """Common code between instruction formats and instruction patterns"""
-    def __init__(self, name, lineno, base, fixb, fixm, udfm, fldm, flds):
+    def __init__(self, name, lineno, base, fixb, fixm, udfm, fldm, flds, chkfs):
         self.name = name
         self.file = input_file
         self.lineno = lineno
@@ -430,6 +430,7 @@ class General:
         self.undefmask = udfm
         self.fieldmask = fldm
         self.fields = flds
+        self.check_funcs = chkfs
 
     def __str__(self):
         r = self.name
@@ -480,6 +481,8 @@ class Pattern(General):
             output(ind, self.base.extract_name(), '(&u.f_', arg, ', insn);\n')
         for n, f in self.fields.items():
             output(ind, 'u.f_', arg, '.', n, ' = ', f.str_extract(), ';\n')
+        for f, a in self.check_funcs:
+            output(ind, 'check_', f, '(ctx, ', a, ');\n')
         output(ind, 'return ', translate_prefix, '_', self.name,
                '(ctx, &u.f_', arg, ');\n')
 # end Pattern
@@ -583,6 +586,11 @@ def add_field_byname(lineno, flds, new_name, old_name):
     return add_field(lineno, flds, new_name, lookup_field(lineno, old_name))
 
 
+def add_func_check(lineno, chkfns, check_funcname, check_arg):
+    chkfns += [(check_funcname, check_arg)]
+    return chkfns
+
+
 def infer_argument_set(flds):
     global arguments
     global decode_function
@@ -624,7 +632,7 @@ def infer_format(arg, fieldmask, flds):
     if not arg:
         arg = infer_argument_set(flds)
 
-    fmt = Format(name, 0, arg, 0, 0, 0, fieldmask, var_flds)
+    fmt = Format(name, 0, arg, 0, 0, 0, fieldmask, var_flds, [])
     formats[name] = fmt
 
     return (fmt, const_flds)
@@ -646,6 +654,7 @@ def parse_generic(lineno, is_format, name, toks):
     undefmask = 0
     width = 0
     flds = {}
+    chkfns = []
     arg = None
     fmt = None
     for t in toks:
@@ -690,6 +699,13 @@ def parse_generic(lineno, is_format, name, toks):
             flds = add_field(lineno, flds, fname, ConstField(value))
             continue
 
+        # '>FUNC=ARG' calls check_FUNC(ctx, ARG).
+        if t[0] == '>':
+            (fname, farg) = t[1:].split('=')
+            tt = t[2 + len(fname) + len(farg):]
+            chkfns = add_func_check(lineno, chkfns, fname, farg)
+            continue
+
         # Pattern of 0s, 1s, dots and dashes indicate required zeros,
         # required ones, or dont-cares.
         if re_fullmatch('[01.-]+', t):
@@ -752,7 +768,7 @@ def parse_generic(lineno, is_format, name, toks):
         if name in formats:
             error(lineno, 'duplicate format name', name)
         fmt = Format(name, lineno, arg, fixedbits, fixedmask,
-                     undefmask, fieldmask, flds)
+                     undefmask, fieldmask, flds, chkfns)
         formats[name] = fmt
     else:
         # Patterns can reference a format ...
@@ -779,7 +795,7 @@ def parse_generic(lineno, is_format, name, toks):
             if f not in flds.keys() and f not in fmt.fields.keys():
                 error(lineno, 'field {0} not initialized'.format(f))
         pat = Pattern(name, lineno, fmt, fixedbits, fixedmask,
-                      undefmask, fieldmask, flds)
+                      undefmask, fieldmask, flds, chkfns)
         patterns.append(pat)
 
     # Validate the masks that we have assembled.
-- 
2.17.2

[Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/Makefile.objs   |  8 ++++++++
 target/mips/insns.decode    |  2 ++
 target/mips/translate.c     |  7 +++++++
 target/mips/translate.inc.c | 13 +++++++++++++
 4 files changed, 30 insertions(+)
 create mode 100644 target/mips/insns.decode
 create mode 100644 target/mips/translate.inc.c

diff --git a/target/mips/Makefile.objs b/target/mips/Makefile.objs
index 651f36f517..3510835d57 100644
--- a/target/mips/Makefile.objs
+++ b/target/mips/Makefile.objs
@@ -2,3 +2,11 @@ obj-y += translate.o dsp_helper.o op_helper.o lmi_helper.o helper.o cpu.o
 obj-y += gdbstub.o msa_helper.o mips-semi.o
 obj-$(CONFIG_SOFTMMU) += machine.o cp0_timer.o
 obj-$(CONFIG_KVM) += kvm.o
+
+DECODETREE = $(SRC_PATH)/scripts/decodetree.py
+
+target/mips/decode.inc.c: $(SRC_PATH)/target/mips/insns.decode $(DECODETREE)
+	$(call quiet-command,\
+	  $(PYTHON) $(DECODETREE) -o $@ $<, "GEN", $(TARGET_DIR)$@)
+
+target/mips/translate.o: target/mips/decode.inc.c
diff --git a/target/mips/insns.decode b/target/mips/insns.decode
new file mode 100644
index 0000000000..7fbf21cbb9
--- /dev/null
+++ b/target/mips/insns.decode
@@ -0,0 +1,2 @@
+# MIPS32/MIPS64 Instruction Set
+#
diff --git a/target/mips/translate.c b/target/mips/translate.c
index e726f3ec00..560325c563 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -27848,6 +27848,8 @@ static void gen_msa(CPUMIPSState *env, DisasContext *ctx)
 
 }
 
+#include "translate.inc.c"
+
 static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
 {
     int32_t offset;
@@ -27872,6 +27874,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
         gen_set_label(l1);
     }
 
+    /* Transition to the auto-generated decoder.  */
+    if (decode(ctx, ctx->opcode)) {
+        return;
+    }
+
     op = MASK_OP_MAJOR(ctx->opcode);
     rs = (ctx->opcode >> 21) & 0x1f;
     rt = (ctx->opcode >> 16) & 0x1f;
diff --git a/target/mips/translate.inc.c b/target/mips/translate.inc.c
new file mode 100644
index 0000000000..69fe78ac89
--- /dev/null
+++ b/target/mips/translate.inc.c
@@ -0,0 +1,13 @@
+/*
+ *  MIPS emulation for QEMU - MIPS32 translation routines
+ *
+ *  Copyright (c) 2004-2005 Jocelyn Mayer
+ *  Copyright (c) 2006 Marius Groeger (FPU operations)
+ *  Copyright (c) 2006 Thiemo Seufer (MIPS32R2 support)
+ *  Copyright (c) 2018 Philippe Mathieu-Daudé
+ *
+ * This code is licensed under the GNU GPLv2 and later.
+ */
+
+/* Include the auto-generated decoder.  */
+#include "decode.inc.c"
-- 
2.17.2

[Qemu-devel] [RFC PATCH 09/11] target/mips: Port SYNCI to decodetree

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/insns.decode    | 8 ++++++++
 target/mips/translate.c     | 6 ------
 target/mips/translate.inc.c | 7 +++++++
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/target/mips/insns.decode b/target/mips/insns.decode
index 7fbf21cbb9..8a1a7acf3a 100644
--- a/target/mips/insns.decode
+++ b/target/mips/insns.decode
@@ -1,2 +1,10 @@
 # MIPS32/MIPS64 Instruction Set
 #
+# From:
+# - MIPS32 Architecture For Programmers Volume II-A (Document Number: MD00086)
+
+####
+# System Instructions
+####
+
+synci           000001 ----- 11111 ----------------          >insn=ISA_MIPS32R2
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 560325c563..760cca8262 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -27948,12 +27948,6 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
             check_insn(ctx, ISA_MIPS32R6);
             generate_exception_end(ctx, EXCP_RI);
             break;
-        case OPC_SYNCI:
-            check_insn(ctx, ISA_MIPS32R2);
-            /* Break the TB to be able to sync copied instructions
-               immediately */
-            ctx->base.is_jmp = DISAS_STOP;
-            break;
         case OPC_BPOSGE32:    /* MIPS DSP branch */
 #if defined(TARGET_MIPS64)
         case OPC_BPOSGE64:
diff --git a/target/mips/translate.inc.c b/target/mips/translate.inc.c
index 69fe78ac89..f3dcd32f98 100644
--- a/target/mips/translate.inc.c
+++ b/target/mips/translate.inc.c
@@ -11,3 +11,10 @@
 
 /* Include the auto-generated decoder.  */
 #include "decode.inc.c"
+
+static bool trans_synci(DisasContext *dc, arg_synci *a)
+{
+    /* Break the TB to be able to sync copied instructions immediately */
+    dc->base.is_jmp = DISAS_STOP;
+    return true;
+}
-- 
2.17.2

[Qemu-devel] [RFC PATCH 10/11] scripts/decodetree: Add add_cond_check()

[Philippe Mathieu-Daudé]

The '?' token allow to check for a condition.

This is useful to take the translate the instruction only if the
condition is valid.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 scripts/decodetree.py | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/scripts/decodetree.py b/scripts/decodetree.py
index 2450cc1a63..ba53ee589e 100755
--- a/scripts/decodetree.py
+++ b/scripts/decodetree.py
@@ -420,7 +420,7 @@ class Arguments:
 
 class General:
     """Common code between instruction formats and instruction patterns"""
-    def __init__(self, name, lineno, base, fixb, fixm, udfm, fldm, flds, chkfs):
+    def __init__(self, name, lineno, base, fixb, fixm, udfm, fldm, flds, chkfs, chkif):
         self.name = name
         self.file = input_file
         self.lineno = lineno
@@ -431,6 +431,7 @@ class General:
         self.fieldmask = fldm
         self.fields = flds
         self.check_funcs = chkfs
+        self.check_cond = chkif
 
     def __str__(self):
         r = self.name
@@ -483,6 +484,9 @@ class Pattern(General):
             output(ind, 'u.f_', arg, '.', n, ' = ', f.str_extract(), ';\n')
         for f, a in self.check_funcs:
             output(ind, 'check_', f, '(ctx, ', a, ');\n')
+        if self.check_cond:
+            output(ind, 'if (!(', self.check_cond, '))\n')
+            output(ind, '    return false;\n')
         output(ind, 'return ', translate_prefix, '_', self.name,
                '(ctx, &u.f_', arg, ');\n')
 # end Pattern
@@ -591,6 +595,10 @@ def add_func_check(lineno, chkfns, check_funcname, check_arg):
     return chkfns
 
 
+def add_cond_check(lineno, chkifs, condition):
+    return condition
+
+
 def infer_argument_set(flds):
     global arguments
     global decode_function
@@ -632,7 +640,7 @@ def infer_format(arg, fieldmask, flds):
     if not arg:
         arg = infer_argument_set(flds)
 
-    fmt = Format(name, 0, arg, 0, 0, 0, fieldmask, var_flds, [])
+    fmt = Format(name, 0, arg, 0, 0, 0, fieldmask, var_flds, [], None)
     formats[name] = fmt
 
     return (fmt, const_flds)
@@ -655,6 +663,7 @@ def parse_generic(lineno, is_format, name, toks):
     width = 0
     flds = {}
     chkfns = []
+    chkifs = None
     arg = None
     fmt = None
     for t in toks:
@@ -706,6 +715,13 @@ def parse_generic(lineno, is_format, name, toks):
             chkfns = add_func_check(lineno, chkfns, fname, farg)
             continue
 
+        # '?condition' calls if(condition).
+        if t[0] == '?':
+            cond = t[1:]
+            tt = t[1 + len(cond):]
+            chkifs = add_cond_check(lineno, chkifs, cond)
+            continue
+
         # Pattern of 0s, 1s, dots and dashes indicate required zeros,
         # required ones, or dont-cares.
         if re_fullmatch('[01.-]+', t):
@@ -768,7 +784,7 @@ def parse_generic(lineno, is_format, name, toks):
         if name in formats:
             error(lineno, 'duplicate format name', name)
         fmt = Format(name, lineno, arg, fixedbits, fixedmask,
-                     undefmask, fieldmask, flds, chkfns)
+                     undefmask, fieldmask, flds, chkfns, chkifs)
         formats[name] = fmt
     else:
         # Patterns can reference a format ...
@@ -795,7 +811,7 @@ def parse_generic(lineno, is_format, name, toks):
             if f not in flds.keys() and f not in fmt.fields.keys():
                 error(lineno, 'field {0} not initialized'.format(f))
         pat = Pattern(name, lineno, fmt, fixedbits, fixedmask,
-                      undefmask, fieldmask, flds, chkfns)
+                      undefmask, fieldmask, flds, chkfns, chkifs)
         patterns.append(pat)
 
     # Validate the masks that we have assembled.
-- 
2.17.2

[Qemu-devel] [RFC PATCH 11/11] target/mips: Port MIPS64 DCL[Z/O] to decodetree

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/insns.decode    | 12 ++++++++++++
 target/mips/translate.inc.c | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/target/mips/insns.decode b/target/mips/insns.decode
index 8a1a7acf3a..e256220211 100644
--- a/target/mips/insns.decode
+++ b/target/mips/insns.decode
@@ -2,9 +2,21 @@
 #
 # From:
 # - MIPS32 Architecture For Programmers Volume II-A (Document Number: MD00086)
+# - MIPS64 Architecture For Programmers Volume II-A (Document Number: MD00087)
+
+&rs_rt_rd       rs rt rd
+
+@rs_rt_rd       ...... rs:5  rt:5  rd:5  00000 ......   &rs_rt_rd
 
 ####
 # System Instructions
 ####
 
 synci           000001 ----- 11111 ----------------          >insn=ISA_MIPS32R2
+
+####
+# Special2 Instructions
+####
+
+dclz            011100 ..... ..... ..... ..... 100100   @rs_rt_rd   ?ctx->insn_flags&ISA_MIPS64
+dclo            011100 ..... ..... ..... ..... 100101   @rs_rt_rd   ?ctx->insn_flags&ISA_MIPS64
diff --git a/target/mips/translate.inc.c b/target/mips/translate.inc.c
index f3dcd32f98..90fe868605 100644
--- a/target/mips/translate.inc.c
+++ b/target/mips/translate.inc.c
@@ -18,3 +18,15 @@ static bool trans_synci(DisasContext *dc, arg_synci *a)
     dc->base.is_jmp = DISAS_STOP;
     return true;
 }
+
+static bool trans_dclz(DisasContext *ctx, arg_rs_rt_rd *a)
+{
+    gen_cl(ctx, OPC_DCLZ, a->rd, a->rs);
+    return true;
+}
+
+static bool trans_dclo(DisasContext *ctx, arg_rs_rt_rd *a)
+{
+    gen_cl(ctx, OPC_DCLO, a->rd, a->rs);
+    return true;
+}
-- 
2.17.2

Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub

[Aleksandar Markovic]

> Subject: [RFC PATCH 08/11] target/mips: Add a decodetree stub

There is no plan to use decodetree for MIPS target. MIPS decoding engine is mostly stable mature code that was well tested over many years, and there is no point in introducing such drastic change to the code that works.

Thanks,
Aleksandar

________________________________________
From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> on behalf of Philippe Mathieu-Daudé <f4bug@amsat.org>
Sent: Monday, November 12, 2018 12:36:19 AM
To: Bastian Koppelmann; Peer Adelt; Richard Henderson
Cc: Philippe Mathieu-Daudé; qemu-devel@nongnu.org; Aurelien Jarno; Aleksandar Markovic
Subject: [RFC PATCH 08/11] target/mips: Add a decodetree stub

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/Makefile.objs   |  8 ++++++++
 target/mips/insns.decode    |  2 ++
 target/mips/translate.c     |  7 +++++++
 target/mips/translate.inc.c | 13 +++++++++++++
 4 files changed, 30 insertions(+)
 create mode 100644 target/mips/insns.decode
 create mode 100644 target/mips/translate.inc.c

diff --git a/target/mips/Makefile.objs b/target/mips/Makefile.objs
index 651f36f517..3510835d57 100644
--- a/target/mips/Makefile.objs
+++ b/target/mips/Makefile.objs
@@ -2,3 +2,11 @@ obj-y += translate.o dsp_helper.o op_helper.o lmi_helper.o helper.o cpu.o
 obj-y += gdbstub.o msa_helper.o mips-semi.o
 obj-$(CONFIG_SOFTMMU) += machine.o cp0_timer.o
 obj-$(CONFIG_KVM) += kvm.o
+
+DECODETREE = $(SRC_PATH)/scripts/decodetree.py
+
+target/mips/decode.inc.c: $(SRC_PATH)/target/mips/insns.decode $(DECODETREE)
+       $(call quiet-command,\
+         $(PYTHON) $(DECODETREE) -o $@ $<, "GEN", $(TARGET_DIR)$@)
+
+target/mips/translate.o: target/mips/decode.inc.c
diff --git a/target/mips/insns.decode b/target/mips/insns.decode
new file mode 100644
index 0000000000..7fbf21cbb9
--- /dev/null
+++ b/target/mips/insns.decode
@@ -0,0 +1,2 @@
+# MIPS32/MIPS64 Instruction Set
+#
diff --git a/target/mips/translate.c b/target/mips/translate.c
index e726f3ec00..560325c563 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -27848,6 +27848,8 @@ static void gen_msa(CPUMIPSState *env, DisasContext *ctx)

 }

+#include "translate.inc.c"
+
 static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
 {
     int32_t offset;
@@ -27872,6 +27874,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
         gen_set_label(l1);
     }

+    /* Transition to the auto-generated decoder.  */
+    if (decode(ctx, ctx->opcode)) {
+        return;
+    }
+
     op = MASK_OP_MAJOR(ctx->opcode);
     rs = (ctx->opcode >> 21) & 0x1f;
     rt = (ctx->opcode >> 16) & 0x1f;
diff --git a/target/mips/translate.inc.c b/target/mips/translate.inc.c
new file mode 100644
index 0000000000..69fe78ac89
--- /dev/null
+++ b/target/mips/translate.inc.c
@@ -0,0 +1,13 @@
+/*
+ *  MIPS emulation for QEMU - MIPS32 translation routines
+ *
+ *  Copyright (c) 2004-2005 Jocelyn Mayer
+ *  Copyright (c) 2006 Marius Groeger (FPU operations)
+ *  Copyright (c) 2006 Thiemo Seufer (MIPS32R2 support)
+ *  Copyright (c) 2018 Philippe Mathieu-Daudé
+ *
+ * This code is licensed under the GNU GPLv2 and later.
+ */
+
+/* Include the auto-generated decoder.  */
+#include "decode.inc.c"
--
2.17.2

Re: [Qemu-devel] [RFC PATCH 03/11] target/mips: Move the !ISA_MIPS32R6 check out of decode_opc_special2_legacy

[Richard Henderson]

On 11/12/18 12:36 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/mips/translate.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RFC PATCH 04/11] target/mips: Avoid access to CPUMIPSState from decode* functions

[Richard Henderson]

On 11/12/18 12:36 AM, Philippe Mathieu-Daudé wrote:
> The DisasContext is already initialized from the CPUMIPSState in
> mips_tr_init_disas_context().
> 
> Suggested-by: Richard Henderson <rth@twiddle.net>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/mips/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Richard Henderson]

On 11/12/18 12:36 AM, Philippe Mathieu-Daudé wrote:
> Python internal representation is signed, so unsigned values
> bigger than 31-bit are interpreted as signed (and printed with
> a '-' signed).
> Mask out to force unsigned values.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> TODO: display error encountered:
> 
>    case 0x-1:
>        ....
> ---
>  scripts/decodetree.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

This must have been from a trivial input file containing only fixed field
insns?  Not saying it's wrong, but just so we know.  Any chance you can build a
tests/decode/ test for this?  I know they're all currently related to parsing,
but...

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RFC PATCH 06/11] scripts/decodetree: Allow empty specifications

[Richard Henderson]

On 11/12/18 12:36 AM, Philippe Mathieu-Daudé wrote:
> Starting with empty specifications allow to write stubs/templates,
> useful when testing/rebasing.
> 
> This fixes:
> 
>   decode.inc.c: In function ‘decode’:
>   decode.inc.c:9:7: error: unused variable ‘u’ [-Werror=unused-variable]
>      } u;
>        ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  scripts/decodetree.py | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RFC PATCH 07/11] scripts/decodetree: Add add_func_check()

[Richard Henderson]

On 11/12/18 12:36 AM, Philippe Mathieu-Daudé wrote:
>          for n, f in self.fields.items():
>              output(ind, 'u.f_', arg, '.', n, ' = ', f.str_extract(), ';\n')
> +        for f, a in self.check_funcs:
> +            output(ind, 'check_', f, '(ctx, ', a, ');\n')
>          output(ind, 'return ', translate_prefix, '_', self.name,
>                 '(ctx, &u.f_', arg, ');\n')

I don't see the value in asserting the isa here.
If this were a conditional check, which might fall through to either another
pattern or to return false, maybe.


r~

Re: [Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Philippe Mathieu-Daudé]

On Mon, Nov 12, 2018 at 9:20 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 11/12/18 12:36 AM, Philippe Mathieu-Daudé wrote:
> > Python internal representation is signed, so unsigned values
> > bigger than 31-bit are interpreted as signed (and printed with
> > a '-' signed).
> > Mask out to force unsigned values.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> > TODO: display error encountered:
> >
> >    case 0x-1:
> >        ....
> > ---
> >  scripts/decodetree.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> This must have been from a trivial input file containing only fixed field
> insns?  Not saying it's wrong, but just so we know.  Any chance you can build a
> tests/decode/ test for this?  I know they're all currently related to parsing,
> but...

Yes, it is in my TODO to add this test, I have that failure in an early branch.
There is no rush with this series for now, this is not 3.2 material.

> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

Phil.

Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub

[Richard Henderson]

On 11/12/18 6:37 AM, Aleksandar Markovic wrote:
>> Subject: [RFC PATCH 08/11] target/mips: Add a decodetree stub
> 
> There is no plan to use decodetree for MIPS target. MIPS decoding engine is
> mostly stable mature code that was well tested over many years, and there is no
> point in introducing such drastic change to the code that works.

This attitude is unnecessarily obstructionist.

The reorganization of the instruction implementation that is implied by a
transition to decodetree is exactly what you and I talked about some months ago.

The ability to use multiple decodetree parsers to vector directly to the
instruction implementations would help unify code across multiple encoding
schemes.  (There 4 now: mips, mips16, micromips, nanomips; and mipsr6 is
different enough that it could be considered a 5th encoding.)

In a sample transition of 10 insns that Phillipe sent me via private email this
weekend, I noticed an existing bug in MULT{,U} that fails to enforce rd == 0 on
cpus that have only one accumulator -- any bets there are no more to be found?
 You have just this year added yet another encoding scheme for nanomips.  Your
statement "well tested over many years" is a bit of a stretch.

If you do not wish to work on this, that's your prerogative.  But I don't think
you should be arbitrarily shutting down experimentation on this line before it
has a chance to show results.


r~

Re: [Qemu-devel] [RFC PATCH 11/11] target/mips: Port MIPS64 DCL[Z/O] to decodetree

[Richard Henderson]

On 11/12/18 12:36 AM, Philippe Mathieu-Daudé wrote:
> +dclz            011100 ..... ..... ..... ..... 100100   @rs_rt_rd   ?ctx->insn_flags&ISA_MIPS64
> +dclo            011100 ..... ..... ..... ..... 100101   @rs_rt_rd   ?ctx->insn_flags&ISA_MIPS64

This syntax I do not like.  I preferred your other form,

  >isa(ISA_MIPS64)

which, with a change to return false as I suggested, would
have the same effect.

As an aside, you could do

#define check_isa(ctx, which)  ((ctx)->insn_flags & ISA_##which)

to reduce the decode markup to

  >isa(MIPS64)


r~

Re: [Qemu-devel] [RFC PATCH 11/11] target/mips: Port MIPS64 DCL[Z/O] to decodetree

[Philippe Mathieu-Daudé]

On Mon, Nov 12, 2018 at 10:15 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/12/18 12:36 AM, Philippe Mathieu-Daudé wrote:
> > +dclz            011100 ..... ..... ..... ..... 100100   @rs_rt_rd   ?ctx->insn_flags&ISA_MIPS64
> > +dclo            011100 ..... ..... ..... ..... 100101   @rs_rt_rd   ?ctx->insn_flags&ISA_MIPS64
>
> This syntax I do not like.  I preferred your other form,
>
>   >isa(ISA_MIPS64)
>
> which, with a change to return false as I suggested, would
> have the same effect.
>
> As an aside, you could do
>
> #define check_isa(ctx, which)  ((ctx)->insn_flags & ISA_##which)
>
> to reduce the decode markup to
>
>   >isa(MIPS64)

Very good idea! Thanks :)

Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub

[Aleksandar Markovic]

Hello, Richard.

I am a little taken aback by your tone. I hope we can communicate in much friendlier maner, as we used to do.

>You have just this year added yet another encoding scheme for nanomips. Your statement "well tested over many years" is a bit of a stretch.

I wrote "mostly stable mature", not "all stable mature".

>If you do not wish to work on this, that's your prerogative.  But I don't think
you should be arbitrarily shutting down experimentation on this line before it
has a chance to show results.

I am not preventing anyone from experimenting. I just want to warn Philippe about high-level view that the code in question, although not the nicest, works, and is planned to be maintained with minimal changes. The focus of MIPS target is on adding new architectures and ASEs, and (I correct myself) it could be that decodetree would kick in in such cases - but not for mature code driving older architectures. It just doesn't make enough sense.

Thanks,
Aleksandar

________________________________________
From: Richard Henderson <richard.henderson@linaro.org>
Sent: Monday, November 12, 2018 9:58:05 AM
To: Aleksandar Markovic; Philippe Mathieu-Daudé; Bastian Koppelmann; Peer Adelt; Richard Henderson
Cc: qemu-devel@nongnu.org; Aurelien Jarno
Subject: Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub

On 11/12/18 6:37 AM, Aleksandar Markovic wrote:
>> Subject: [RFC PATCH 08/11] target/mips: Add a decodetree stub
>
> There is no plan to use decodetree for MIPS target. MIPS decoding engine is
> mostly stable mature code that was well tested over many years, and there is no
> point in introducing such drastic change to the code that works.

This attitude is unnecessarily obstructionist.

The reorganization of the instruction implementation that is implied by a
transition to decodetree is exactly what you and I talked about some months ago.

The ability to use multiple decodetree parsers to vector directly to the
instruction implementations would help unify code across multiple encoding
schemes.  (There 4 now: mips, mips16, micromips, nanomips; and mipsr6 is
different enough that it could be considered a 5th encoding.)

In a sample transition of 10 insns that Phillipe sent me via private email this
weekend, I noticed an existing bug in MULT{,U} that fails to enforce rd == 0 on
cpus that have only one accumulator -- any bets there are no more to be found?
 You have just this year added yet another encoding scheme for nanomips.  Your
statement "well tested over many years" is a bit of a stretch.

If you do not wish to work on this, that's your prerogative.  But I don't think
you should be arbitrarily shutting down experimentation on this line before it
has a chance to show results.


r~

Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub

[Richard Henderson]

On 11/12/18 11:04 AM, Aleksandar Markovic wrote:
> Hello, Richard.
> 
> I am a little taken aback by your tone. I hope we can communicate in much friendlier maner, as we used to do.

I too was put off by your tone.  Beginning with "there is no plan" and
continuing with "there is no point" is a curt reply.  It is off-putting to
someone attempting to contribute, and I felt it was unwarranted.

> I am not preventing anyone from experimenting. I just want to warn Philippe about high-level view that the code in question, although not the nicest, works, and is planned to be maintained with minimal changes. The focus of MIPS target is on adding new architectures and ASEs, and (I correct myself) it could be that decodetree would kick in in such cases - but not for mature code driving older architectures. It just doesn't make enough sense.

I think this is a mistake.  The legacy code is quite tangled.  It *should* have
an overhaul.  If someone is willing to do that work, fantastic.


r~

Re: [Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Eduardo Habkost]

On Mon, Nov 12, 2018 at 12:36:16AM +0100, Philippe Mathieu-Daudé wrote:
> Python internal representation is signed, so unsigned values
> bigger than 31-bit are interpreted as signed (and printed with
> a '-' signed).
> Mask out to force unsigned values.

I don't understand this commit description. Python surely
supports integers larger than 2^31, and its internal
representation shouldn't matter at all:

  >>> '0x{0:08x}'.format(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
  '0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'

Can you explain how the code ends up with a negative value in the
`self.thismask` or `b` variables?  If `self.subs` contains
negative values, this is likely to break other parts of the code.

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> TODO: display error encountered:
> 
>    case 0x-1:
>        ....

How can I reproduce it?

> ---
>  scripts/decodetree.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> index 5dea15e7a5..08aa52d544 100755
> --- a/scripts/decodetree.py
> +++ b/scripts/decodetree.py
> @@ -900,12 +900,12 @@ class Tree:
>              def str_case(b):
>                  return '0x{0:08x}'.format(b)
>  
> -        output(ind, 'switch (', str_switch(self.thismask), ') {\n')
> +        output(ind, 'switch (', str_switch(self.thismask & insnmask), ') {\n')
>          for b, s in sorted(self.subs):
>              assert (self.thismask & ~s.fixedmask) == 0
>              innermask = outermask | self.thismask
>              innerbits = outerbits | b
> -            output(ind, 'case ', str_case(b), ':\n')
> +            output(ind, 'case ', str_case(b & insnmask), ':\n')
>              output(ind, '    /* ',
>                     str_match_bits(innerbits, innermask), ' */\n')
>              s.output_code(i + 4, extracted, innerbits, innermask)
> -- 
> 2.17.2
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Philippe Mathieu-Daudé]

On Mon, Nov 12, 2018 at 6:03 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Nov 12, 2018 at 12:36:16AM +0100, Philippe Mathieu-Daudé wrote:
> > Python internal representation is signed, so unsigned values
> > bigger than 31-bit are interpreted as signed (and printed with
> > a '-' signed).
> > Mask out to force unsigned values.
>
> I don't understand this commit description. Python surely
> supports integers larger than 2^31, and its internal
> representation shouldn't matter at all:
>
>   >>> '0x{0:08x}'.format(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
>   '0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'
>
> Can you explain how the code ends up with a negative value in the
> `self.thismask` or `b` variables?  If `self.subs` contains
> negative values, this is likely to break other parts of the code.

I guess I misunderstood the error, thus the description is invalid.

>
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> > TODO: display error encountered:
> >
> >    case 0x-1:
> >        ....
>
> How can I reproduce it?

$ scripts/decodetree.py /dev/null

    switch (insn & 0x-0000001) {
    }

main() ->
  build_tree(patterns, 0, outermask = 0) ->
       innermask = ~outermask # = -1
      Tree(fullmask, innermask = -1) ->
        __init__(self, fm, tm = -1) ->
          self.thismask = tm # -1
  t.output_code(4, False, 0, 0) ->
    sh = is_contiguous(self.thismask) # -1
        output(ind, 'switch (', str_switch(self.thismask), ') {\n')

        with:

            def str_switch(b):
                return 'insn & 0x{0:08x}'.format(b)

So the fix is rather:

-- >8 --
diff --git a/scripts/decodetree.py b/scripts/decodetree.py
@@ -916,7 +916,7 @@ class Tree:

def build_tree(pats, outerbits, outermask):
     # Find the intersection of all remaining fixedmask.
-    innermask = ~outermask
+    innermask = ~outermask & insnmask
     for i in pats:
         innermask &= i.fixedmask
 ---

Re: [Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Philippe Mathieu-Daudé]

> On Mon, Nov 12, 2018 at 6:03 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Nov 12, 2018 at 12:36:16AM +0100, Philippe Mathieu-Daudé wrote:
> > > Python internal representation is signed, so unsigned values
> > > bigger than 31-bit are interpreted as signed (and printed with
> > > a '-' signed).
> > > Mask out to force unsigned values.
> >
> > I don't understand this commit description. Python surely
> > supports integers larger than 2^31, and its internal
> > representation shouldn't matter at all:
> >
> >   >>> '0x{0:08x}'.format(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
> >   '0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'

  >>> '0x{0:08x}'.format(-1)
  '0x-0000001'

> >
> > Can you explain how the code ends up with a negative value in the
> > `self.thismask` or `b` variables?  If `self.subs` contains
> > negative values, this is likely to break other parts of the code.
>
> I guess I misunderstood the error, thus the description is invalid.
>
> >
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > > TODO: display error encountered:
> > >
> > >    case 0x-1:
> > >        ....
> >
> > How can I reproduce it?
>
> $ scripts/decodetree.py /dev/null
>
>     switch (insn & 0x-0000001) {
>     }
>
> main() ->
>   build_tree(patterns, 0, outermask = 0) ->
>        innermask = ~outermask # = -1
>       Tree(fullmask, innermask = -1) ->
>         __init__(self, fm, tm = -1) ->
>           self.thismask = tm # -1
>   t.output_code(4, False, 0, 0) ->
>     sh = is_contiguous(self.thismask) # -1
>         output(ind, 'switch (', str_switch(self.thismask), ') {\n')
>
>         with:
>
>             def str_switch(b):
>                 return 'insn & 0x{0:08x}'.format(b)
>
> So the fix is rather:
>
> -- >8 --
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> @@ -916,7 +916,7 @@ class Tree:
>
> def build_tree(pats, outerbits, outermask):
>      # Find the intersection of all remaining fixedmask.
> -    innermask = ~outermask
> +    innermask = ~outermask & insnmask
>      for i in pats:
>          innermask &= i.fixedmask
>  ---

Re: [Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Richard Henderson]

On 11/12/18 7:52 PM, Philippe Mathieu-Daudé wrote:
> So the fix is rather:
> 
> -- >8 --
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> @@ -916,7 +916,7 @@ class Tree:
> 
> def build_tree(pats, outerbits, outermask):
>      # Find the intersection of all remaining fixedmask.
> -    innermask = ~outermask
> +    innermask = ~outermask & insnmask
>      for i in pats:
>          innermask &= i.fixedmask
>  ---

Ah!  I like it.


r~

Re: [Qemu-devel] [RFC PATCH 05/11] decodetree: Force Python to print unsigned values

[Eduardo Habkost]

On Mon, Nov 12, 2018 at 07:52:28PM +0100, Philippe Mathieu-Daudé wrote:
> On Mon, Nov 12, 2018 at 6:03 PM Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Nov 12, 2018 at 12:36:16AM +0100, Philippe Mathieu-Daudé wrote:
> > > Python internal representation is signed, so unsigned values
> > > bigger than 31-bit are interpreted as signed (and printed with
> > > a '-' signed).
> > > Mask out to force unsigned values.
> >
> > I don't understand this commit description. Python surely
> > supports integers larger than 2^31, and its internal
> > representation shouldn't matter at all:
> >
> >   >>> '0x{0:08x}'.format(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
> >   '0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'
> >
> > Can you explain how the code ends up with a negative value in the
> > `self.thismask` or `b` variables?  If `self.subs` contains
> > negative values, this is likely to break other parts of the code.
> 
> I guess I misunderstood the error, thus the description is invalid.
> 
> >
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > > TODO: display error encountered:
> > >
> > >    case 0x-1:
> > >        ....
> >
> > How can I reproduce it?
> 
> $ scripts/decodetree.py /dev/null
> 
>     switch (insn & 0x-0000001) {
>     }
> 
> main() ->
>   build_tree(patterns, 0, outermask = 0) ->
>        innermask = ~outermask # = -1
>       Tree(fullmask, innermask = -1) ->
>         __init__(self, fm, tm = -1) ->
>           self.thismask = tm # -1
>   t.output_code(4, False, 0, 0) ->
>     sh = is_contiguous(self.thismask) # -1
>         output(ind, 'switch (', str_switch(self.thismask), ') {\n')
> 
>         with:
> 
>             def str_switch(b):
>                 return 'insn & 0x{0:08x}'.format(b)
> 
> So the fix is rather:
> 
> -- >8 --
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> @@ -916,7 +916,7 @@ class Tree:
> 
> def build_tree(pats, outerbits, outermask):
>      # Find the intersection of all remaining fixedmask.
> -    innermask = ~outermask
> +    innermask = ~outermask & insnmask
>      for i in pats:
>          innermask &= i.fixedmask

Nice.  Thanks for the explanation.

This really seems to be the only place in the code where the bit
operations may generate a negative number.  All remaining usage
of the ~ operator in the code is already inside & expressions
that will ensure the result is positive.

To the alternative fix above:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 02/11] decodetree: Add multiple include guard

[Eduardo Habkost]

On Mon, Nov 12, 2018 at 12:36:13AM +0100, Philippe Mathieu-Daudé wrote:
> It is necessary when splitting an ISA, or when using multiple ISAs.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> TODO: explain why, use case
> TODO: escape full path?
> ---
>  scripts/decodetree.py | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
> index 0bc73b5990..5dea15e7a5 100755
> --- a/scripts/decodetree.py
> +++ b/scripts/decodetree.py
> @@ -1030,7 +1030,11 @@ def main():
>      else:
>          output_fd = sys.stdout
>  
> +    hdr_guard = filename.split(os.path.sep)[-1].upper().replace('.', '_') + "_H"

Isn't
  filename.split(os.path.sep)[-1]
equivalent to
  os.path.basename(filename)
?


> +    hdr_guard = filename.split(os.path.sep)[-1].upper().replace('.', '_') + "_H"
> +
>      output_autogen()
> +    output('#ifndef ' + hdr_guard + '\n')
> +    output('#define ' + hdr_guard + '\n')
>      for n in sorted(arguments.keys()):
>          f = arguments[n]
>          f.output_def()
> @@ -1066,6 +1070,7 @@ def main():
>      t.output_code(4, False, 0, 0)
>  
>      output('}\n')
> +    output('#endif /* ' + hdr_guard + ' */\n')
>  
>      if output_file:
>          output_fd.close()
> -- 
> 2.17.2
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 02/11] decodetree: Add multiple include guard

[Philippe Mathieu-Daudé]

On 12/11/18 23:30, Eduardo Habkost wrote:
> On Mon, Nov 12, 2018 at 12:36:13AM +0100, Philippe Mathieu-Daudé wrote:
>> It is necessary when splitting an ISA, or when using multiple ISAs.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> TODO: explain why, use case
>> TODO: escape full path?
>> ---
>>   scripts/decodetree.py | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/scripts/decodetree.py b/scripts/decodetree.py
>> index 0bc73b5990..5dea15e7a5 100755
>> --- a/scripts/decodetree.py
>> +++ b/scripts/decodetree.py
>> @@ -1030,7 +1030,11 @@ def main():
>>       else:
>>           output_fd = sys.stdout
>>   
>> +    hdr_guard = filename.split(os.path.sep)[-1].upper().replace('.', '_') + "_H"
> 
> Isn't
>    filename.split(os.path.sep)[-1]
> equivalent to
>    os.path.basename(filename)
> ?

Yes, thanks :)