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

2024-01-15 Thread ltaylorsimpson



> -Original Message-
> From: Brian Cain 
> Sent: Sunday, January 14, 2024 5:21 PM
> To: Taylor Simpson ; qemu-devel@nongnu.org
> Cc: Matheus Bernardino (QUIC) ; Sid
> Manning ; Marco Liebel (QUIC)
> ; 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 
> > Sent: Monday, January 8, 2024 4:49 PM
> > To: qemu-devel@nongnu.org
> > Cc: Brian Cain ; Matheus Bernardino (QUIC)
> > ; Sid Manning ;
> Marco
> > Liebel (QUIC) ;
> > 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.pyGenerate 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 
> > ---
> >  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 +

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

2024-01-14 Thread Brian Cain


> -Original Message-
> From: Taylor Simpson 
> Sent: Monday, January 8, 2024 4:49 PM
> To: qemu-devel@nongnu.org
> Cc: Brian Cain ; Matheus Bernardino (QUIC)
> ; Sid Manning ; Marco
> Liebel (QUIC) ; 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)
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> 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.pyGenerate 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 
> ---
>  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
> 
> diff --git a/target/hexagon/decode.h b/target/hexagon/decode.h
> index c66f5ea64d..3f3012b978 100644
> --- a/target/hexagon/decode.h
> +++ b/target/hexagon/decode.h
> @@ -21,12 +21,13 @@
>  #include "cpu.h"
>  #include "opcodes.h"
>  #include "insn.h"
> +#include "translate.h"
> 
>  void decode_init(void);
> 
>  void decode_send_insn_to(Packet *packet, int start, int newloc);
> 
> -int decode_packet(int max_words, const uint32_t *words, Packet *pkt,
> -  bool disas_only);
> +int decode_packet(DisasContext *ctx, int max_words, const uint32_t *words,
> +  Packet *pkt, bool disas_only);
> 
>  #endif
> diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
> index 946c55cc71..bddad1f75e 100644
> --- a/target/hexagon/decode.c
> +++ b/target/hexagon/decode.c
> @@ -52,6 +52,34 @@ DEF_REGMAP(R_8,   8,  0, 1, 2, 3, 4, 5, 6, 7)
>  #define DECODE_MAPPED_REG(OPNUM, NAME) \
>  insn->regno[OPNUM] = DECODE_REGISTER_##NAME[insn->regno[OPNUM]];
> 
> +/* Helper functions for decode_*_generated.c.inc */
> +#define DECODE_MAPPED(NAME) \
> +static int decode_mapped_reg_##NAME(DisasContext *ctx, int x) \
> +{ \
> +return DECODE_REGISTER_##NAME[x]; \
> +}
> +DECODE_MAPPED(R_16)
> +DECODE_MAPPED(R_8)
> +
> +/* Helper function for decodetree_trans_funcs_generated.c.inc */
> +static int shift_left(DisasContext *ctx, int x, int n, int immno)
> +{
> +int ret = x;
> +Insn *insn = ctx->insn;
> +if (!insn->extension_valid ||
> +insn->which_extended != immno) {
> +ret <<= n;
> +}
> +return ret;
> +}
> +
> +/* Include the generated decoder for 32 bit insn */
> +#include "decode_normal_generated.c.inc"
> +#include "decode_hvx_generated.c.inc"
> +
> +/* Include the generated helpers for the decoder */
> +#include "decodetree_trans_funcs_generated.c.inc"
> +
>  typedef struct {
>  const struct DectreeTable *table_link;
>  const struct DectreeTable *table_link_b;
> @@ -550,7 +578,8 @@ apply_extender(Packet *pkt, int i, uint32_t extender)
>  int immed_num;
>  uint32_t base_immed;
> 
> -immed_num = opcode_which_immediate_is_extended(pkt->insn[i].opcode);
> +immed_num = pkt->insn[i].which_extended;
> +g_assert(immed_num == opcode_which_immediate_is_extended(pkt-
> >insn[i].opcode));
>  base_immed = pkt->insn[i].immed[immed_num];
> 
>  pkt->insn[i].immed[immed_num] = extender | fZXTN(6, 32, base_immed);
> @@ -762,12 +791,19 @@ decode_insns_tablewalk(Insn *insn, const
> DectreeTable *table,
>  }
> 
>  static unsigned int
> -decode_insns(Insn *insn, uint32_t encoding)
> +decode_insns(DisasContext *ctx, Insn *insn, uint32_t encoding)
>  {
>  const DectreeTable *table;
>  if (parse_bits(encoding) != 0) {
> +if (decode_normal(ctx, encoding) ||
> +decode_hvx(ctx, encoding)) {
> +insn->generate = opcode_genptr[insn->opcode];
> +insn->iclass =