> -----Original Message-----
> From: Brian Cain <bc...@quicinc.com>
> Sent: Sunday, January 14, 2024 5:21 PM
> To: Taylor Simpson <ltaylorsimp...@gmail.com>; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) <quic_mathb...@quicinc.com>; Sid
> Manning <sidn...@quicinc.com>; Marco Liebel (QUIC)
> <quic_mlie...@quicinc.com>; richard.hender...@linaro.org;
> phi...@linaro.org; a...@rev.ng; a...@rev.ng
> Subject: RE: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU
> decodetree (32-bit instructions)
>
>
>
> > -----Original Message-----
> > From: Taylor Simpson <ltaylorsimp...@gmail.com>
> > Sent: Monday, January 8, 2024 4:49 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain <bc...@quicinc.com>; Matheus Bernardino (QUIC)
> > <quic_mathb...@quicinc.com>; Sid Manning <sidn...@quicinc.com>;
> Marco
> > Liebel (QUIC) <quic_mlie...@quicinc.com>;
> > richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng;
> > a...@rev.ng; ltaylorsimp...@gmail.com
> > Subject: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU decodetree
> > (32- bit instructions)
> >
> >
> > The Decodetree Specification can be found here
> > https://www.qemu.org/docs/master/devel/decodetree.html
> >
> > Covers all 32-bit instructions, including HVX
> >
> > We generate separate decoders for each instruction class. The reason
> > will be more apparent in the next patch in this series.
> >
> > We add 2 new scripts
> > gen_decodetree.py Generate the input to decodetree.py
> > gen_trans_funcs.py Generate the trans_* functions used by the
> > output of decodetree.py
> >
> > Since the functions generated by decodetree.py take DisasContext * as
> > an argument, we add the argument to a couple of functions that didn't
> > need it previously. We also set the insn field in DisasContext during
> > decode because it is used by the trans_* functions.
> >
> > There is a g_assert_not_reached() in decode_insns() in decode.c to
> > verify we never try to use the old decoder on 32-bit instructions
> >
> > Signed-off-by: Taylor Simpson <ltaylorsimp...@gmail.com>
> > ---
> > target/hexagon/decode.h | 5 +-
> > target/hexagon/decode.c | 54 ++++++++-
> > target/hexagon/translate.c | 4 +-
> > target/hexagon/README | 13 +-
> > target/hexagon/gen_decodetree.py | 193
> > ++++++++++++++++++++++++++++++
> target/hexagon/gen_trans_funcs.py | 132 ++++++++++++++++++++
> > target/hexagon/meson.build | 55 +++++++++
> > 7 files changed, 442 insertions(+), 14 deletions(-) create mode
> > 100755 target/hexagon/gen_decodetree.py create mode 100755
> > target/hexagon/gen_trans_funcs.py
> >
>
> LGTM, but some nitpicky suggestions:
>
> diff --git a/target/hexagon/gen_decodetree.py
> b/target/hexagon/gen_decodetree.py
> index 2dff975f55..62bd8a62b6 100755
> --- a/target/hexagon/gen_decodetree.py
> +++ b/target/hexagon/gen_decodetree.py
> @@ -57,7 +57,7 @@ def ordered_unique(l):
> "d",
> "e",
> "f",
> - "g"
> + "g",
> }
>
> #
> @@ -104,9 +104,6 @@ def gen_decodetree_file(f, class_to_decode):
> if skip_tag(tag, class_to_decode):
> continue
>
> - f.write("########################################")
> - f.write("########################################\n")
> -
> enc = encs[tag]
> enc_str = "".join(reversed(encs[tag]))
>
> @@ -115,21 +112,21 @@ def gen_decodetree_file(f, class_to_decode):
> if is_subinsn:
> enc_str = "---" + enc_str
>
> - f.write(f"## {tag}:\t{enc_str}\n")
> - f.write("##\n")
> + f.write(("#" * 80) + "\n"
> + f"## {tag}:\t{enc_str}\n"
> + "##\n")
>
> regs = ordered_unique(regre.findall(iset.iset[tag]["syntax"]))
> imms = ordered_unique(immre.findall(iset.iset[tag]["syntax"]))
>
> # Write the field definitions for the registers
> - regno = 0
> - for reg in regs:
> - reg_type = reg[0]
> - reg_id = reg[1]
> + for regno, reg in enumerate(regs):
> + reg_type, reg_id, _, reg_enc_size = reg
> reg_letter = reg_id[0]
> - reg_num_choices = int(reg[3].rstrip("S"))
> - reg_mapping = reg[0] + "".join(["_" for letter in reg[1]]) +
> reg[3]
> + reg_num_choices = int(reg_enc_size.rstrip("S"))
> + reg_mapping = reg_type + "".join("_" for letter in reg_id)
> + + reg_enc_size
> reg_enc_fields = re.findall(reg_letter + "+", enc)
> + print(f'{reg} -> {reg_enc_fields}')
>
> # Check for some errors
> if len(reg_enc_fields) == 0:
> @@ -140,13 +137,12 @@ def gen_decodetree_file(f, class_to_decode):
> if 2 ** len(reg_enc_field) != reg_num_choices:
> raise Exception(f"{tag} has incorrect register field width!")
>
> - f.write(f"%{tag}_{reg_type}{reg_id}\t")
> - f.write(f"{enc.index(reg_enc_field)}:{len(reg_enc_field)}")
> + f.write(f"%{tag}_{reg_type}{reg_id}\t"
> + f"{enc.index(reg_enc_field)}:{len(reg_enc_field)}")
> if (reg_type in num_registers and
> reg_num_choices != num_registers[reg_type]):
> f.write(f"\t!function=decode_mapped_reg_{reg_mapping}")
> f.write("\n")
> - regno += 1
>
> # Write the field definitions for the immediates
> for imm in imms:
> @@ -189,8 +185,7 @@ def gen_decodetree_file(f, class_to_decode):
> f.write("\n")
>
> # Replace the 0s and 1s with .
> - for x in { "0", "1" }:
> - enc_str = enc_str.replace(x, ".")
> + enc_str = enc_str.replace("0", ".").replace("1", ".")
>
> # Write the instruction pattern
> f.write(f"{tag}\t{enc_str} @{tag}\n")
>
>
> Consider some or all of the above, but regardless --
>
> Reviewed-by: Brian Cain <bc...@quicinc.com>
Thanks,
I will make the changes you suggest.
Taylor
