RE: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU decodetree (32-bit instructions)

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <bcain@quicinc.com>
> Sent: Sunday, January 14, 2024 5:21 PM
> To: Taylor Simpson <ltaylorsimpson@gmail.com>; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) <quic_mathbern@quicinc.com>; Sid
> Manning <sidneym@quicinc.com>; Marco Liebel (QUIC)
> <quic_mliebel@quicinc.com>; richard.henderson@linaro.org;
> philmd@linaro.org; ale@rev.ng; anjo@rev.ng
> Subject: RE: [PATCH v2 1/3] Hexagon (target/hexagon) Use QEMU
> decodetree (32-bit instructions)
> 
> 
> 
> > -----Original Message-----
> > From: Taylor Simpson <ltaylorsimpson@gmail.com>
> > Sent: Monday, January 8, 2024 4:49 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> > <quic_mathbern@quicinc.com>; Sid Manning <sidneym@quicinc.com>;
> Marco
> > Liebel (QUIC) <quic_mliebel@quicinc.com>;
> > richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng;
> > anjo@rev.ng; ltaylorsimpson@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 <ltaylorsimpson@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 <bcain@quicinc.com>

Thanks,
I will make the changes you suggest.

Taylor