[PATCH 0/4] target/hexagon: Minor idef-parser cleanup

[PATCH 0/4] target/hexagon: Minor idef-parser cleanup

[Anton Johansson via]

Was running idef-parser with valgrind and noticed we were leaking the
init_list GArray, which is used to hold instruction arguments that may
need initialization.  This patchset fixes the leak, removes unused
macros and undefined functions, and simplifies gen_inst_init_args() to
only handle predicate values.

Anton Johansson (4):
  target/hexagon: idef-parser remove unused defines
  target/hexagon: idef-parser remove undefined functions
  target/hexagon: idef-parser fix leak of init_list
  target/hexagon: idef-parser simplify predicate init

 target/hexagon/idef-parser/idef-parser.h    | 10 --------
 target/hexagon/idef-parser/idef-parser.y    |  2 --
 target/hexagon/idef-parser/parser-helpers.c | 27 ++++++++++++---------
 target/hexagon/idef-parser/parser-helpers.h | 13 ----------
 4 files changed, 16 insertions(+), 36 deletions(-)

-- 
2.44.0

[PATCH 1/4] target/hexagon: idef-parser remove unused defines

[Anton Johansson via]

Before switching to GArray/g_string_printf we used fixed size arrays for
output buffers and instructions arguments among other things.

Macros defining the sizes of these buffers were left behind, remove
them.

Signed-off-by: Anton Johansson <anjo@rev.ng>
---
 target/hexagon/idef-parser/idef-parser.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/target/hexagon/idef-parser/idef-parser.h b/target/hexagon/idef-parser/idef-parser.h
index 3faa1deecd..8594cbe3a2 100644
--- a/target/hexagon/idef-parser/idef-parser.h
+++ b/target/hexagon/idef-parser/idef-parser.h
@@ -23,16 +23,6 @@
 #include <stdbool.h>
 #include <glib.h>
 
-#define TCGV_NAME_SIZE 7
-#define MAX_WRITTEN_REGS 32
-#define OFFSET_STR_LEN 32
-#define ALLOC_LIST_LEN 32
-#define ALLOC_NAME_SIZE 32
-#define INIT_LIST_LEN 32
-#define OUT_BUF_LEN (1024 * 1024)
-#define SIGNATURE_BUF_LEN (128 * 1024)
-#define HEADER_BUF_LEN (128 * 1024)
-
 /* Variadic macros to wrap the buffer printing functions */
 #define EMIT(c, ...)                                                           \
     do {                                                                       \
-- 
2.44.0

RE: [PATCH 1/4] target/hexagon: idef-parser remove unused defines

[ltaylorsimpson]

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Monday, May 6, 2024 1:31 PM
> To: qemu-devel@nongnu.org
> Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@quicinc.com
> Subject: [PATCH 1/4] target/hexagon: idef-parser remove unused defines
> 
> Before switching to GArray/g_string_printf we used fixed size arrays for
> output buffers and instructions arguments among other things.
> 
> Macros defining the sizes of these buffers were left behind, remove them.
> 
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>  target/hexagon/idef-parser/idef-parser.h | 10 ----------
>  1 file changed, 10 deletions(-)

Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>

[PATCH 2/4] target/hexagon: idef-parser remove undefined functions

[Anton Johansson via]

Signed-off-by: Anton Johansson <anjo@rev.ng>
---
 target/hexagon/idef-parser/parser-helpers.h | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/target/hexagon/idef-parser/parser-helpers.h b/target/hexagon/idef-parser/parser-helpers.h
index 7c58087169..2087d534a9 100644
--- a/target/hexagon/idef-parser/parser-helpers.h
+++ b/target/hexagon/idef-parser/parser-helpers.h
@@ -143,8 +143,6 @@ void commit(Context *c);
 
 #define OUT(c, locp, ...) FOR_EACH((c), (locp), OUT_IMPL, __VA_ARGS__)
 
-const char *cmp_swap(Context *c, YYLTYPE *locp, const char *type);
-
 /**
  * Temporary values creation
  */
@@ -236,8 +234,6 @@ HexValue gen_extract_op(Context *c,
                         HexValue *index,
                         HexExtract *extract);
 
-HexValue gen_read_reg(Context *c, YYLTYPE *locp, HexValue *reg);
-
 void gen_write_reg(Context *c, YYLTYPE *locp, HexValue *reg, HexValue *value);
 
 void gen_assign(Context *c,
@@ -274,13 +270,6 @@ HexValue gen_ctpop_op(Context *c, YYLTYPE *locp, HexValue *src);
 
 HexValue gen_rotl(Context *c, YYLTYPE *locp, HexValue *src, HexValue *n);
 
-HexValue gen_deinterleave(Context *c, YYLTYPE *locp, HexValue *mixed);
-
-HexValue gen_interleave(Context *c,
-                        YYLTYPE *locp,
-                        HexValue *odd,
-                        HexValue *even);
-
 HexValue gen_carry_from_add(Context *c,
                             YYLTYPE *locp,
                             HexValue *op1,
@@ -349,8 +338,6 @@ HexValue gen_rvalue_ternary(Context *c, YYLTYPE *locp, HexValue *cond,
 
 const char *cond_to_str(TCGCond cond);
 
-void emit_header(Context *c);
-
 void emit_arg(Context *c, YYLTYPE *locp, HexValue *arg);
 
 void emit_footer(Context *c);
-- 
2.44.0

RE: [PATCH 2/4] target/hexagon: idef-parser remove undefined functions

[ltaylorsimpson]

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Monday, May 6, 2024 1:31 PM
> To: qemu-devel@nongnu.org
> Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@quicinc.com
> Subject: [PATCH 2/4] target/hexagon: idef-parser remove undefined
> functions
> 
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>  target/hexagon/idef-parser/parser-helpers.h | 13 -------------
>  1 file changed, 13 deletions(-)

Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>

[PATCH 3/4] target/hexagon: idef-parser fix leak of init_list

[Anton Johansson via]

gen_inst_init_args() is called for instructions using a predicate as an
rvalue. Upon first call, the list of arguments which might need
initialization init_list is freed to indicate that they have been
processed. For instructions without an rvalue predicate,
gen_inst_init_args() isn't called and init_list will never be freed.

Free init_list from free_instruction() if it hasn't already been freed.

Signed-off-by: Anton Johansson <anjo@rev.ng>
---
 target/hexagon/idef-parser/parser-helpers.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target/hexagon/idef-parser/parser-helpers.c b/target/hexagon/idef-parser/parser-helpers.c
index 95f2b43076..bae01c2bb8 100644
--- a/target/hexagon/idef-parser/parser-helpers.c
+++ b/target/hexagon/idef-parser/parser-helpers.c
@@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
         g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
     }
     g_array_free(c->inst.strings, TRUE);
+    /*
+     * Free list of arguments that might need initialization, if they haven't
+     * already been free'd.
+     */
+    if (c->inst.init_list) {
+        g_array_free(c->inst.init_list, TRUE);
+    }
     /* Free INAME token value */
     g_string_free(c->inst.name, TRUE);
     /* Free variables and registers */
-- 
2.44.0

RE: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list

[ltaylorsimpson]

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Monday, May 6, 2024 1:31 PM
> To: qemu-devel@nongnu.org
> Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@quicinc.com
> Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> 
> gen_inst_init_args() is called for instructions using a predicate as an
rvalue.
> Upon first call, the list of arguments which might need initialization
init_list is
> freed to indicate that they have been processed. For instructions without
an
> rvalue predicate,
> gen_inst_init_args() isn't called and init_list will never be freed.
> 
> Free init_list from free_instruction() if it hasn't already been freed.
> 
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>  target/hexagon/idef-parser/parser-helpers.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/target/hexagon/idef-parser/parser-helpers.c
> b/target/hexagon/idef-parser/parser-helpers.c
> index 95f2b43076..bae01c2bb8 100644
> --- a/target/hexagon/idef-parser/parser-helpers.c
> +++ b/target/hexagon/idef-parser/parser-helpers.c
> @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
>          g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
>      }
>      g_array_free(c->inst.strings, TRUE);
> +    /*
> +     * Free list of arguments that might need initialization, if they
haven't
> +     * already been free'd.
> +     */
> +    if (c->inst.init_list) {
> +        g_array_free(c->inst.init_list, TRUE);
> +    }
>      /* Free INAME token value */
>      g_string_free(c->inst.name, TRUE);
>      /* Free variables and registers */

Why not do this in gen_inst_init_args just before this?
   /* Free argument init list once we have initialized everything */
    g_array_free(c->inst.init_list, TRUE);
    c->inst.init_list = NULL;


Taylor

Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list

['Anton Johansson' via]

On 06/05/24, ltaylorsimpson@gmail.com wrote:
> 
> 
> > -----Original Message-----
> > From: Anton Johansson <anjo@rev.ng>
> > Sent: Monday, May 6, 2024 1:31 PM
> > To: qemu-devel@nongnu.org
> > Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@quicinc.com
> > Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> > 
> > gen_inst_init_args() is called for instructions using a predicate as an
> rvalue.
> > Upon first call, the list of arguments which might need initialization
> init_list is
> > freed to indicate that they have been processed. For instructions without
> an
> > rvalue predicate,
> > gen_inst_init_args() isn't called and init_list will never be freed.
> > 
> > Free init_list from free_instruction() if it hasn't already been freed.
> > 
> > Signed-off-by: Anton Johansson <anjo@rev.ng>
> > ---
> >  target/hexagon/idef-parser/parser-helpers.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/target/hexagon/idef-parser/parser-helpers.c
> > b/target/hexagon/idef-parser/parser-helpers.c
> > index 95f2b43076..bae01c2bb8 100644
> > --- a/target/hexagon/idef-parser/parser-helpers.c
> > +++ b/target/hexagon/idef-parser/parser-helpers.c
> > @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
> >          g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
> >      }
> >      g_array_free(c->inst.strings, TRUE);
> > +    /*
> > +     * Free list of arguments that might need initialization, if they
> haven't
> > +     * already been free'd.
> > +     */
> > +    if (c->inst.init_list) {
> > +        g_array_free(c->inst.init_list, TRUE);
> > +    }
> >      /* Free INAME token value */
> >      g_string_free(c->inst.name, TRUE);
> >      /* Free variables and registers */
> 
> Why not do this in gen_inst_init_args just before this?
>    /* Free argument init list once we have initialized everything */
>     g_array_free(c->inst.init_list, TRUE);
>     c->inst.init_list = NULL;

Thanks for the reviews Taylor! I'm not sure I understand what you mean
here, we already free init_list in gen_inst_init_args, since
gen_inst_init_args won't be called for all instructions we need to also
free from a common place.

//Anton

RE: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list

[ltaylorsimpson]

> -----Original Message-----
> From: 'Anton Johansson' <anjo@rev.ng>
> Sent: Tuesday, May 7, 2024 4:47 AM
> To: ltaylorsimpson@gmail.com
> Cc: qemu-devel@nongnu.org; ale@rev.ng; bcain@quicinc.com
> Subject: Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> 
> On 06/05/24, ltaylorsimpson@gmail.com wrote:
> >
> >
> > > -----Original Message-----
> > > From: Anton Johansson <anjo@rev.ng>
> > > Sent: Monday, May 6, 2024 1:31 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@quicinc.com
> > > Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of
> > > init_list
> > >
> > > gen_inst_init_args() is called for instructions using a predicate as
> > > an
> > rvalue.
> > > Upon first call, the list of arguments which might need
> > > initialization
> > init_list is
> > > freed to indicate that they have been processed. For instructions
> > > without
> > an
> > > rvalue predicate,
> > > gen_inst_init_args() isn't called and init_list will never be freed.
> > >
> > > Free init_list from free_instruction() if it hasn't already been freed.
> > >
> > > Signed-off-by: Anton Johansson <anjo@rev.ng>
> > > ---
> > >  target/hexagon/idef-parser/parser-helpers.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/target/hexagon/idef-parser/parser-helpers.c
> > > b/target/hexagon/idef-parser/parser-helpers.c
> > > index 95f2b43076..bae01c2bb8 100644
> > > --- a/target/hexagon/idef-parser/parser-helpers.c
> > > +++ b/target/hexagon/idef-parser/parser-helpers.c
> > > @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
> > >          g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
> > >      }
> > >      g_array_free(c->inst.strings, TRUE);
> > > +    /*
> > > +     * Free list of arguments that might need initialization, if
> > > + they
> > haven't
> > > +     * already been free'd.
> > > +     */
> > > +    if (c->inst.init_list) {
> > > +        g_array_free(c->inst.init_list, TRUE);
> > > +    }
> > >      /* Free INAME token value */
> > >      g_string_free(c->inst.name, TRUE);
> > >      /* Free variables and registers */
> >
> > Why not do this in gen_inst_init_args just before this?
> >    /* Free argument init list once we have initialized everything */
> >     g_array_free(c->inst.init_list, TRUE);
> >     c->inst.init_list = NULL;
> 
> Thanks for the reviews Taylor! I'm not sure I understand what you mean
> here, we already free init_list in gen_inst_init_args, since gen_inst_init_args
> won't be called for all instructions we need to also free from a common
> place.
> 
> //Anton

It just seems more natural to free the elements of the array at the same place you free the array itself.  If there are valid reasons for doing it elsewhere, I'm OK with that.

Taylor

Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list

['Anton Johansson' via]

On 07/05/24, ltaylorsimpson@gmail.com wrote:
> 
> 
> > -----Original Message-----
> > From: 'Anton Johansson' <anjo@rev.ng>
> > Sent: Tuesday, May 7, 2024 4:47 AM
> > To: ltaylorsimpson@gmail.com
> > Cc: qemu-devel@nongnu.org; ale@rev.ng; bcain@quicinc.com
> > Subject: Re: [PATCH 3/4] target/hexagon: idef-parser fix leak of init_list
> > 
> > On 06/05/24, ltaylorsimpson@gmail.com wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Anton Johansson <anjo@rev.ng>
> > > > Sent: Monday, May 6, 2024 1:31 PM
> > > > To: qemu-devel@nongnu.org
> > > > Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@quicinc.com
> > > > Subject: [PATCH 3/4] target/hexagon: idef-parser fix leak of
> > > > init_list
> > > >
> > > > gen_inst_init_args() is called for instructions using a predicate as
> > > > an
> > > rvalue.
> > > > Upon first call, the list of arguments which might need
> > > > initialization
> > > init_list is
> > > > freed to indicate that they have been processed. For instructions
> > > > without
> > > an
> > > > rvalue predicate,
> > > > gen_inst_init_args() isn't called and init_list will never be freed.
> > > >
> > > > Free init_list from free_instruction() if it hasn't already been freed.
> > > >
> > > > Signed-off-by: Anton Johansson <anjo@rev.ng>
> > > > ---
> > > >  target/hexagon/idef-parser/parser-helpers.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/target/hexagon/idef-parser/parser-helpers.c
> > > > b/target/hexagon/idef-parser/parser-helpers.c
> > > > index 95f2b43076..bae01c2bb8 100644
> > > > --- a/target/hexagon/idef-parser/parser-helpers.c
> > > > +++ b/target/hexagon/idef-parser/parser-helpers.c
> > > > @@ -2121,6 +2121,13 @@ void free_instruction(Context *c)
> > > >          g_string_free(g_array_index(c->inst.strings, GString*, i), TRUE);
> > > >      }
> > > >      g_array_free(c->inst.strings, TRUE);
> > > > +    /*
> > > > +     * Free list of arguments that might need initialization, if
> > > > + they
> > > haven't
> > > > +     * already been free'd.
> > > > +     */
> > > > +    if (c->inst.init_list) {
> > > > +        g_array_free(c->inst.init_list, TRUE);
> > > > +    }
> > > >      /* Free INAME token value */
> > > >      g_string_free(c->inst.name, TRUE);
> > > >      /* Free variables and registers */
> > >
> > > Why not do this in gen_inst_init_args just before this?
> > >    /* Free argument init list once we have initialized everything */
> > >     g_array_free(c->inst.init_list, TRUE);
> > >     c->inst.init_list = NULL;
> > 
> > Thanks for the reviews Taylor! I'm not sure I understand what you mean
> > here, we already free init_list in gen_inst_init_args, since gen_inst_init_args
> > won't be called for all instructions we need to also free from a common
> > place.
> > 
> > //Anton
> 
> It just seems more natural to free the elements of the array at the same place you free the array itself.  If there are valid reasons for doing it elsewhere, I'm OK with that.
> 
> Taylor
> 
> 

Ah I see what you mean. The array and its elements are either freed in
gen_inst_init_args or free_instruction so they do occur together. The
"freeing of variables and registers" comment only refers to declared
TCGvs and has nothing to do with the arguments.

Comment is a bit outdated so I've updated it.

//Anton

[PATCH 4/4] target/hexagon: idef-parser simplify predicate init

[Anton Johansson via]

Only predicate instruction arguments need to be initialized by
idef-parser. This commit removes registers from the init_list and
simplifies gen_inst_init_args() slightly.

Signed-off-by: Anton Johansson <anjo@rev.ng>
---
 target/hexagon/idef-parser/idef-parser.y    |  2 --
 target/hexagon/idef-parser/parser-helpers.c | 20 +++++++++-----------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/target/hexagon/idef-parser/idef-parser.y b/target/hexagon/idef-parser/idef-parser.y
index cd2612eb8c..9ffb9f9699 100644
--- a/target/hexagon/idef-parser/idef-parser.y
+++ b/target/hexagon/idef-parser/idef-parser.y
@@ -233,8 +233,6 @@ code : '{' statements '}'
 argument_decl : REG
                 {
                     emit_arg(c, &@1, &$1);
-                    /* Enqueue register into initialization list */
-                    g_array_append_val(c->inst.init_list, $1);
                 }
               | PRED
                 {
diff --git a/target/hexagon/idef-parser/parser-helpers.c b/target/hexagon/idef-parser/parser-helpers.c
index bae01c2bb8..33e8f82007 100644
--- a/target/hexagon/idef-parser/parser-helpers.c
+++ b/target/hexagon/idef-parser/parser-helpers.c
@@ -1652,26 +1652,24 @@ void gen_inst(Context *c, GString *iname)
 
 
 /*
- * Initialize declared but uninitialized registers, but only for
- * non-conditional instructions
+ * Initialize declared but uninitialized instruction arguments. Only needed for
+ * predicate arguments, initialization of registers is handled by the Hexagon
+ * frontend.
  */
 void gen_inst_init_args(Context *c, YYLTYPE *locp)
 {
+    /* If init_list is NULL arguments have already been initialized */
     if (!c->inst.init_list) {
         return;
     }
 
     for (unsigned i = 0; i < c->inst.init_list->len; i++) {
         HexValue *val = &g_array_index(c->inst.init_list, HexValue, i);
-        if (val->type == REGISTER_ARG) {
-            /* Nothing to do here */
-        } else if (val->type == PREDICATE) {
-            char suffix = val->is_dotnew ? 'N' : 'V';
-            EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
-                      val->pred.id, suffix);
-        } else {
-            yyassert(c, locp, false, "Invalid arg type!");
-        }
+        yyassert(c, locp, val->type == PREDICATE,
+                 "Only predicates need to be initialized!");
+        char suffix = val->is_dotnew ? 'N' : 'V';
+        EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
+                  val->pred.id, suffix);
     }
 
     /* Free argument init list once we have initialized everything */
-- 
2.44.0

RE: [PATCH 4/4] target/hexagon: idef-parser simplify predicate init

[ltaylorsimpson]

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Monday, May 6, 2024 1:31 PM
> To: qemu-devel@nongnu.org
> Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@quicinc.com
> Subject: [PATCH 4/4] target/hexagon: idef-parser simplify predicate init
> 
> Only predicate instruction arguments need to be initialized by
idef-parser.
> This commit removes registers from the init_list and simplifies
> gen_inst_init_args() slightly.
> 
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>  target/hexagon/idef-parser/idef-parser.y    |  2 --
>  target/hexagon/idef-parser/parser-helpers.c | 20 +++++++++-----------
>  2 files changed, 9 insertions(+), 13 deletions(-)

> diff --git a/target/hexagon/idef-parser/parser-helpers.c
> b/target/hexagon/idef-parser/parser-helpers.c
> index bae01c2bb8..33e8f82007 100644
> --- a/target/hexagon/idef-parser/parser-helpers.c
> +++ b/target/hexagon/idef-parser/parser-helpers.c
> @@ -1652,26 +1652,24 @@ void gen_inst(Context *c, GString *iname)
> 
>  void gen_inst_init_args(Context *c, YYLTYPE *locp)  {
> +    /* If init_list is NULL arguments have already been initialized */
>      if (!c->inst.init_list) {
>          return;
>      }
> 
>      for (unsigned i = 0; i < c->inst.init_list->len; i++) {
>          HexValue *val = &g_array_index(c->inst.init_list, HexValue, i);
> -        if (val->type == REGISTER_ARG) {
> -            /* Nothing to do here */
> -        } else if (val->type == PREDICATE) {
> -            char suffix = val->is_dotnew ? 'N' : 'V';
> -            EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
> -                      val->pred.id, suffix);
> -        } else {
> -            yyassert(c, locp, false, "Invalid arg type!");
> -        }
> +        yyassert(c, locp, val->type == PREDICATE,
> +                 "Only predicates need to be initialized!");
> +        char suffix = val->is_dotnew ? 'N' : 'V';

Declarations should be at the beginning of the function per QEMU coding
standards.

> +        EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,

Since you know this is a predicate, the bit_width will always be 32.  You
can hard-code that instead of using %u.

> +                  val->pred.id, suffix);
>      }
> 
>      /* Free argument init list once we have initialized everything */

Taylor

Re: [PATCH 4/4] target/hexagon: idef-parser simplify predicate init

['Anton Johansson' via]

On 06/05/24, ltaylorsimpson@gmail.com wrote:
> 
> 
> > -----Original Message-----
> > From: Anton Johansson <anjo@rev.ng>
> > Sent: Monday, May 6, 2024 1:31 PM
> > To: qemu-devel@nongnu.org
> > Cc: ale@rev.ng; ltaylorsimpson@gmail.com; bcain@quicinc.com
> > Subject: [PATCH 4/4] target/hexagon: idef-parser simplify predicate init
> > 
> > Only predicate instruction arguments need to be initialized by
> idef-parser.
> > This commit removes registers from the init_list and simplifies
> > gen_inst_init_args() slightly.
> > 
> > Signed-off-by: Anton Johansson <anjo@rev.ng>
> > ---
> >  target/hexagon/idef-parser/idef-parser.y    |  2 --
> >  target/hexagon/idef-parser/parser-helpers.c | 20 +++++++++-----------
> >  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> > diff --git a/target/hexagon/idef-parser/parser-helpers.c
> > b/target/hexagon/idef-parser/parser-helpers.c
> > index bae01c2bb8..33e8f82007 100644
> > --- a/target/hexagon/idef-parser/parser-helpers.c
> > +++ b/target/hexagon/idef-parser/parser-helpers.c
> > @@ -1652,26 +1652,24 @@ void gen_inst(Context *c, GString *iname)
> > 
> >  void gen_inst_init_args(Context *c, YYLTYPE *locp)  {
> > +    /* If init_list is NULL arguments have already been initialized */
> >      if (!c->inst.init_list) {
> >          return;
> >      }
> > 
> >      for (unsigned i = 0; i < c->inst.init_list->len; i++) {
> >          HexValue *val = &g_array_index(c->inst.init_list, HexValue, i);
> > -        if (val->type == REGISTER_ARG) {
> > -            /* Nothing to do here */
> > -        } else if (val->type == PREDICATE) {
> > -            char suffix = val->is_dotnew ? 'N' : 'V';
> > -            EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
> > -                      val->pred.id, suffix);
> > -        } else {
> > -            yyassert(c, locp, false, "Invalid arg type!");
> > -        }
> > +        yyassert(c, locp, val->type == PREDICATE,
> > +                 "Only predicates need to be initialized!");
> > +        char suffix = val->is_dotnew ? 'N' : 'V';
> 
> Declarations should be at the beginning of the function per QEMU coding
> standards.

Agh right!

>
> > +        EMIT_HEAD(c, "tcg_gen_movi_i%u(P%c%c, 0);\n", val->bit_width,
> 
> Since you know this is a predicate, the bit_width will always be 32.  You
> can hard-code that instead of using %u.

Good point, I'll add a paranoia assertion as well.

//Anton