[PATCH v3 0/5] misc hexagon patches

[PATCH v3 0/5] misc hexagon patches

[Brian Cain]

Changes since previous "misc hexagon patches" series (v2):
- changed author to match MAINTAINERS (I was fooled by the mailmap before --
so, for real this time).


Brian Cain (5):
  target/hexagon: handle .new values
  target/hexagon: Fix badva reference, delete CAUSE
  target/hexagon: Add missing A_CALL attr, hintjumpr to multi_cof
  target/hexagon: s/pkt_has_store/pkt_has_scalar_store
  target/hexagon: Remove unreachable

 target/hexagon/idef-parser/README.rst       |  2 +-
 target/hexagon/insn.h                       |  4 +--
 target/hexagon/macros.h                     |  8 +++---
 target/hexagon/cpu.c                        |  3 +--
 target/hexagon/decode.c                     | 10 ++++---
 target/hexagon/genptr.c                     |  3 ++-
 target/hexagon/idef-parser/parser-helpers.c |  4 +--
 target/hexagon/op_helper.c                  |  4 +--
 target/hexagon/translate.c                  |  9 ++++---
 target/hexagon/gen_helper_funcs.py          |  2 +-
 target/hexagon/hex_common.py                | 29 ++++++++++++++++-----
 11 files changed, 49 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] target/hexagon: handle .new values

[Brian Cain]

Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/hex_common.py | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
index 758e5fd12d..6803908718 100755
--- a/target/hexagon/hex_common.py
+++ b/target/hexagon/hex_common.py
@@ -350,6 +350,7 @@ def helper_arg(self):
             f"{self.helper_arg_type()} {self.helper_arg_name()}"
         )
 
+
 #
 # Every register is either Single or Pair or Hvx
 #
@@ -1070,11 +1071,22 @@ def init_registers():
     for reg in new_regs:
         new_registers[f"{reg.regtype}{reg.regid}"] = reg
 
-def get_register(tag, regtype, regid):
-    if f"{regtype}{regid}V" in semdict[tag]:
-        return registers[f"{regtype}{regid}"]
-    else:
-        return new_registers[f"{regtype}{regid}"]
+def is_new_reg(tag, regid):
+    if regid[0] in "NO":
+        return True
+    return regid[0] == "P" and \
+           f"{regid}N" in semdict[tag] and \
+           f"{regid}V" not in semdict[tag]
+
+def get_register(tag, regtype, regid, subtype=""):
+    regid = f"{regtype}{regid}"
+    is_new = is_new_reg(tag, regid)
+    try:
+        reg = new_registers[regid] if is_new else registers[regid]
+    except KeyError:
+        raise Exception(f"Unknown {'new ' if is_new else ''}register {regid}" +\
+                        f"from '{tag}' with syntax '{semdict[tag]}'") from None
+    return reg
 
 def helper_ret_type(tag, regs):
     ## If there is a scalar result, it is the return type
-- 
2.34.1

[PATCH v3 2/5] target/hexagon: Fix badva reference, delete CAUSE

[Brian Cain]

The BADVA reg is referred to with the wrong identifier.  The
CAUSE reg field of SSR is not yet modeled.

Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/cpu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 766b678651..62f1fe15b8 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -216,8 +216,7 @@ static void hexagon_dump(CPUHexagonState *env, FILE *f, int flags)
     qemu_fprintf(f, "  cs0 = 0x00000000\n");
     qemu_fprintf(f, "  cs1 = 0x00000000\n");
 #else
-    print_reg(f, env, HEX_REG_CAUSE);
-    print_reg(f, env, HEX_REG_BADVA);
+    print_reg(f, env, HEX_SREG_BADVA);
     print_reg(f, env, HEX_REG_CS0);
     print_reg(f, env, HEX_REG_CS1);
 #endif
-- 
2.34.1

[PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr to multi_cof

[Brian Cain]

Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/hex_common.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
index 6803908718..a2dcb0aa2e 100755
--- a/target/hexagon/hex_common.py
+++ b/target/hexagon/hex_common.py
@@ -247,8 +247,11 @@ def need_next_PC(tag):
 
 
 def need_pkt_has_multi_cof(tag):
-    return "A_COF" in attribdict[tag]
-
+    return (
+        "A_JUMP" in attribdict[tag]
+        or "A_CALL" in attribdict[tag]
+        or "J2_rte" == tag
+    ) and tag != "J2_hintjumpr"
 
 def need_pkt_need_commit(tag):
     return 'A_IMPLICIT_WRITES_USR' in attribdict[tag]
-- 
2.34.1

[PATCH v3 4/5] target/hexagon: s/pkt_has_store/pkt_has_scalar_store

[Brian Cain]

To remove any confusion with HVX or other potential store instructions,
we'll qualify this context var with "scalar".

Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/idef-parser/README.rst       | 2 +-
 target/hexagon/insn.h                       | 4 ++--
 target/hexagon/macros.h                     | 8 ++++----
 target/hexagon/decode.c                     | 4 ++--
 target/hexagon/genptr.c                     | 3 ++-
 target/hexagon/idef-parser/parser-helpers.c | 4 ++--
 target/hexagon/op_helper.c                  | 4 ++--
 target/hexagon/translate.c                  | 9 +++++----
 target/hexagon/gen_helper_funcs.py          | 2 +-
 9 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/target/hexagon/idef-parser/README.rst b/target/hexagon/idef-parser/README.rst
index 7199177ee3..235e3debee 100644
--- a/target/hexagon/idef-parser/README.rst
+++ b/target/hexagon/idef-parser/README.rst
@@ -637,7 +637,7 @@ tinycode for the Hexagon ``add`` instruction
 ::
 
    ---- 00021094
-   mov_i32 pkt_has_store_s1,$0x0
+   mov_i32 pkt_has_scalar_store_s1,$0x0
    add_i32 tmp0,r2,r2
    mov_i32 loc2,tmp0
    mov_i32 new_r1,loc2
diff --git a/target/hexagon/insn.h b/target/hexagon/insn.h
index 24dcf7fe9f..5d59430da9 100644
--- a/target/hexagon/insn.h
+++ b/target/hexagon/insn.h
@@ -66,8 +66,8 @@ struct Packet {
 
     bool pkt_has_dczeroa;
 
-    bool pkt_has_store_s0;
-    bool pkt_has_store_s1;
+    bool pkt_has_scalar_store_s0;
+    bool pkt_has_scalar_store_s1;
 
     bool pkt_has_hvx;
     Insn *vhist_insn;
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index ee3d4c88e7..b6e5c8aae2 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -82,7 +82,7 @@
  */
 #define CHECK_NOSHUF(VA, SIZE) \
     do { \
-        if (insn->slot == 0 && ctx->pkt->pkt_has_store_s1) { \
+        if (insn->slot == 0 && ctx->pkt->pkt_has_scalar_store_s1) { \
             probe_noshuf_load(VA, SIZE, ctx->mem_idx); \
             process_store(ctx, 1); \
         } \
@@ -93,11 +93,11 @@
         TCGLabel *noshuf_label = gen_new_label(); \
         tcg_gen_brcondi_tl(TCG_COND_EQ, PRED, 0, noshuf_label); \
         GET_EA; \
-        if (insn->slot == 0 && ctx->pkt->pkt_has_store_s1) { \
+        if (insn->slot == 0 && ctx->pkt->pkt_has_scalar_store_s1) { \
             probe_noshuf_load(EA, SIZE, ctx->mem_idx); \
         } \
         gen_set_label(noshuf_label); \
-        if (insn->slot == 0 && ctx->pkt->pkt_has_store_s1) { \
+        if (insn->slot == 0 && ctx->pkt->pkt_has_scalar_store_s1) { \
             process_store(ctx, 1); \
         } \
     } while (0)
@@ -524,7 +524,7 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val, int shift)
 
 #define fLOAD(NUM, SIZE, SIGN, EA, DST) \
     do { \
-        check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+        check_noshuf(env, pkt_has_scalar_store_s1, slot, EA, SIZE, GETPC()); \
         DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE(env, EA, GETPC()); \
     } while (0)
 #endif
diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
index 23deba2426..b5ece60450 100644
--- a/target/hexagon/decode.c
+++ b/target/hexagon/decode.c
@@ -236,9 +236,9 @@ static void decode_set_insn_attr_fields(Packet *pkt)
             if (GET_ATTRIB(opcode, A_SCALAR_STORE) &&
                 !GET_ATTRIB(opcode, A_MEMSIZE_0B)) {
                 if (pkt->insn[i].slot == 0) {
-                    pkt->pkt_has_store_s0 = true;
+                    pkt->pkt_has_scalar_store_s0 = true;
                 } else {
-                    pkt->pkt_has_store_s1 = true;
+                    pkt->pkt_has_scalar_store_s1 = true;
                 }
             }
         }
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index 2c5e15cfcf..7c73772e40 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -395,7 +395,8 @@ static inline void gen_store_conditional8(DisasContext *ctx,
 #ifndef CONFIG_HEXAGON_IDEF_PARSER
 static TCGv gen_slotval(DisasContext *ctx)
 {
-    int slotval = (ctx->pkt->pkt_has_store_s1 & 1) | (ctx->insn->slot << 1);
+    int slotval =
+        (ctx->pkt->pkt_has_scalar_store_s1 & 1) | (ctx->insn->slot << 1);
     return tcg_constant_tl(slotval);
 }
 #endif
diff --git a/target/hexagon/idef-parser/parser-helpers.c b/target/hexagon/idef-parser/parser-helpers.c
index a7dcd85fe4..3316c230f8 100644
--- a/target/hexagon/idef-parser/parser-helpers.c
+++ b/target/hexagon/idef-parser/parser-helpers.c
@@ -1725,7 +1725,7 @@ void gen_cancel(Context *c, YYLTYPE *locp)
 
 void gen_load_cancel(Context *c, YYLTYPE *locp)
 {
-    OUT(c, locp, "if (insn->slot == 0 && pkt->pkt_has_store_s1) {\n");
+    OUT(c, locp, "if (insn->slot == 0 && pkt->pkt_has_scalar_store_s1) {\n");
     OUT(c, locp, "ctx->s1_store_processed = false;\n");
     OUT(c, locp, "process_store(ctx, 1);\n");
     OUT(c, locp, "}\n");
@@ -1750,7 +1750,7 @@ void gen_load(Context *c, YYLTYPE *locp, HexValue *width,
 
     /* Lookup the effective address EA */
     find_variable(c, locp, ea, ea);
-    OUT(c, locp, "if (insn->slot == 0 && pkt->pkt_has_store_s1) {\n");
+    OUT(c, locp, "if (insn->slot == 0 && pkt->pkt_has_scalar_store_s1) {\n");
     OUT(c, locp, "probe_noshuf_load(", ea, ", ", width, ", ctx->mem_idx);\n");
     OUT(c, locp, "process_store(ctx, 1);\n");
     OUT(c, locp, "}\n");
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 6da8db8ea5..6ff37680d9 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -463,11 +463,11 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask)
  * If the load is in slot 0 and there is a store in slot1 (that
  * wasn't cancelled), we have to do the store first.
  */
-static void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+static void check_noshuf(CPUHexagonState *env, bool pkt_has_scalar_store_s1,
                          uint32_t slot, target_ulong vaddr, int size,
                          uintptr_t ra)
 {
-    if (slot == 0 && pkt_has_store_s1 &&
+    if (slot == 0 && pkt_has_scalar_store_s1 &&
         ((env->slot_cancelled & (1 << 1)) == 0)) {
         probe_read(env, vaddr, size, MMU_USER_IDX, ra);
         commit_store(env, 1, ra);
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index 5271c4e022..aca77dfdb1 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -705,11 +705,11 @@ static void process_store_log(DisasContext *ctx)
      *  the memory accesses overlap.
      */
     Packet *pkt = ctx->pkt;
-    if (pkt->pkt_has_store_s1) {
+    if (pkt->pkt_has_scalar_store_s1) {
         g_assert(!pkt->pkt_has_dczeroa);
         process_store(ctx, 1);
     }
-    if (pkt->pkt_has_store_s0) {
+    if (pkt->pkt_has_scalar_store_s0) {
         g_assert(!pkt->pkt_has_dczeroa);
         process_store(ctx, 0);
     }
@@ -834,8 +834,9 @@ static void gen_commit_packet(DisasContext *ctx)
      * involved in committing the packet.
      */
     Packet *pkt = ctx->pkt;
-    bool has_store_s0 = pkt->pkt_has_store_s0;
-    bool has_store_s1 = (pkt->pkt_has_store_s1 && !ctx->s1_store_processed);
+    bool has_store_s0 = pkt->pkt_has_scalar_store_s0;
+    bool has_store_s1 =
+        (pkt->pkt_has_scalar_store_s1 && !ctx->s1_store_processed);
     bool has_hvx_store = pkt_has_hvx_store(pkt);
     if (pkt->pkt_has_dczeroa) {
         /*
diff --git a/target/hexagon/gen_helper_funcs.py b/target/hexagon/gen_helper_funcs.py
index c1f806ac4b..a9c0e27a80 100755
--- a/target/hexagon/gen_helper_funcs.py
+++ b/target/hexagon/gen_helper_funcs.py
@@ -69,7 +69,7 @@ def gen_helper_function(f, tag, tagregs, tagimms):
     if hex_common.need_slot(tag):
         if "A_LOAD" in hex_common.attribdict[tag]:
             f.write(hex_common.code_fmt(f"""\
-                bool pkt_has_store_s1 = slotval & 0x1;
+                bool pkt_has_scalar_store_s1 = slotval & 0x1;
             """))
         f.write(hex_common.code_fmt(f"""\
             uint32_t slot = slotval >> 1;
-- 
2.34.1

[PATCH v3 5/5] target/hexagon: Remove unreachable

[Brian Cain]

We should raise an exception in the event that we encounter a packet
that can't be correctly decoded, not fault.

Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
---
 target/hexagon/decode.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
index b5ece60450..1db7f1950f 100644
--- a/target/hexagon/decode.c
+++ b/target/hexagon/decode.c
@@ -489,7 +489,6 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t encoding)
             insn->iclass = iclass_bits(encoding);
             return 1;
         }
-        g_assert_not_reached();
     } else {
         uint32_t iclass = get_duplex_iclass(encoding);
         unsigned int slot0_subinsn = get_slot0_subinsn(encoding);
@@ -512,6 +511,11 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t encoding)
         }
         g_assert_not_reached();
     }
+    /*
+     * invalid/unrecognized opcode; return 1 and let gen_insn() raise an
+     * exception when it sees this empty insn.
+     */
+    return 1;
 }
 
 static void decode_add_endloop_insn(Insn *insn, int loopnum)
-- 
2.34.1

Re: [PATCH v3 0/5] misc hexagon patches

[Matheus Tavares Bernardino]

On Mon,  7 Apr 2025 12:27:00 -0700 Brian Cain <brian.cain@oss.qualcomm.com> wrote:
> 
> Brian Cain (5):
>   target/hexagon: handle .new values
>   target/hexagon: Fix badva reference, delete CAUSE
>   target/hexagon: Add missing A_CALL attr, hintjumpr to multi_cof
>   target/hexagon: s/pkt_has_store/pkt_has_scalar_store
>   target/hexagon: Remove unreachable

All patches,

Reviewed-by: Matheus Tavares Bernardino <matheus.bernardino@oss.qualcomm.com>

RE: [PATCH v3 1/5] target/hexagon: handle .new values

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Monday, April 7, 2025 1:27 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; matheus.bernardino@oss.qualcomm.com; ale@rev.ng;
> anjo@rev.ng; marco.liebel@oss.qualcomm.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com
> Subject: [PATCH v3 1/5] target/hexagon: handle .new values
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/hex_common.py | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)

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

RE: [PATCH v3 2/5] target/hexagon: Fix badva reference, delete CAUSE

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Monday, April 7, 2025 1:27 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; matheus.bernardino@oss.qualcomm.com; ale@rev.ng;
> anjo@rev.ng; marco.liebel@oss.qualcomm.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com
> Subject: [PATCH v3 2/5] target/hexagon: Fix badva reference, delete CAUSE
> 
> The BADVA reg is referred to with the wrong identifier.  The CAUSE reg field
> of SSR is not yet modeled.
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/cpu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index
> 766b678651..62f1fe15b8 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -216,8 +216,7 @@ static void hexagon_dump(CPUHexagonState *env,
> FILE *f, int flags)
>      qemu_fprintf(f, "  cs0 = 0x00000000\n");
>      qemu_fprintf(f, "  cs1 = 0x00000000\n");  #else
> -    print_reg(f, env, HEX_REG_CAUSE);
> -    print_reg(f, env, HEX_REG_BADVA);
> +    print_reg(f, env, HEX_SREG_BADVA);

Since BADVA is a proxy for BADVA0/BADVA1, consider naming it HEX_SREG_BADVA_ALIASED to help avoid the problems we've seen with HEX_REG_P3_0_ALIASED.

Taylor

RE: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr to multi_cof

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Monday, April 7, 2025 1:27 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; matheus.bernardino@oss.qualcomm.com; ale@rev.ng;
> anjo@rev.ng; marco.liebel@oss.qualcomm.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com
> Subject: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr
> to multi_cof
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/hex_common.py | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/hexagon/hex_common.py
> b/target/hexagon/hex_common.py index 6803908718..a2dcb0aa2e 100755
> --- a/target/hexagon/hex_common.py
> +++ b/target/hexagon/hex_common.py
> @@ -247,8 +247,11 @@ def need_next_PC(tag):
> 
> 
>  def need_pkt_has_multi_cof(tag):
> -    return "A_COF" in attribdict[tag]
> -
> +    return (
> +        "A_JUMP" in attribdict[tag]
> +        or "A_CALL" in attribdict[tag]
> +        or "J2_rte" == tag
> +    ) and tag != "J2_hintjumpr"

It would be better to make this decision with instruction attributes only rather than a mix of attributes and specific tags.  If needed, add another add_qemu_macro_attrib call to hex_common.calculate_attribs.

Having said that, the correct tag for hintjumpr is J*4*_hintjumpr.

Taylor

RE: [PATCH v3 4/5] target/hexagon: s/pkt_has_store/pkt_has_scalar_store

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Monday, April 7, 2025 1:27 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; matheus.bernardino@oss.qualcomm.com; ale@rev.ng;
> anjo@rev.ng; marco.liebel@oss.qualcomm.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com
> Subject: [PATCH v3 4/5] target/hexagon:
> s/pkt_has_store/pkt_has_scalar_store
> 
> To remove any confusion with HVX or other potential store instructions, we'll
> qualify this context var with "scalar".
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/idef-parser/README.rst       | 2 +-
>  target/hexagon/insn.h                       | 4 ++--
>  target/hexagon/macros.h                     | 8 ++++----
>  target/hexagon/decode.c                     | 4 ++--
>  target/hexagon/genptr.c                     | 3 ++-
>  target/hexagon/idef-parser/parser-helpers.c | 4 ++--
>  target/hexagon/op_helper.c                  | 4 ++--
>  target/hexagon/translate.c                  | 9 +++++----
>  target/hexagon/gen_helper_funcs.py          | 2 +-
>  9 files changed, 21 insertions(+), 19 deletions(-)

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

RE: [PATCH v3 5/5] target/hexagon: Remove unreachable

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Monday, April 7, 2025 1:27 PM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; matheus.bernardino@oss.qualcomm.com; ale@rev.ng;
> anjo@rev.ng; marco.liebel@oss.qualcomm.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com
> Subject: [PATCH v3 5/5] target/hexagon: Remove unreachable
> 
> We should raise an exception in the event that we encounter a packet that
> can't be correctly decoded, not fault.
> 
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  target/hexagon/decode.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c index
> b5ece60450..1db7f1950f 100644
> --- a/target/hexagon/decode.c
> +++ b/target/hexagon/decode.c
> @@ -489,7 +489,6 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t
> encoding)
>              insn->iclass = iclass_bits(encoding);
>              return 1;
>          }
> -        g_assert_not_reached();
>      } else {
>          uint32_t iclass = get_duplex_iclass(encoding);
>          unsigned int slot0_subinsn = get_slot0_subinsn(encoding); @@ -512,6
> +511,11 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t encoding)
>          }
>          g_assert_not_reached();

Why leave this one rather than raising an exception?

>      }
> +    /*
> +     * invalid/unrecognized opcode; return 1 and let gen_insn() raise an
> +     * exception when it sees this empty insn.
> +     */
> +    return 1;

You should set insn->generate to NULL if you want to guarantee that gen_insn will raise an exception.  A better option is to return a special value that indicates "invalid" and have decode_packet return 0 which will cause decode_and_translate_packet to generate the exception before generating the code for any other instructions in the packet.

Do you have a test case for this?

Taylor

Re: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr to multi_cof

[Brian Cain]

On 4/14/2025 12:04 PM, ltaylorsimpson@gmail.com wrote:
>
>> -----Original Message-----
>> From: Brian Cain <brian.cain@oss.qualcomm.com>
>> Sent: Monday, April 7, 2025 1:27 PM
>> To: qemu-devel@nongnu.org
>> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
>> philmd@linaro.org; matheus.bernardino@oss.qualcomm.com; ale@rev.ng;
>> anjo@rev.ng; marco.liebel@oss.qualcomm.com; ltaylorsimpson@gmail.com;
>> alex.bennee@linaro.org; quic_mburton@quicinc.com;
>> sidneym@quicinc.com
>> Subject: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr
>> to multi_cof
>>
>> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
>> ---
>>   target/hexagon/hex_common.py | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/hexagon/hex_common.py
>> b/target/hexagon/hex_common.py index 6803908718..a2dcb0aa2e 100755
>> --- a/target/hexagon/hex_common.py
>> +++ b/target/hexagon/hex_common.py
>> @@ -247,8 +247,11 @@ def need_next_PC(tag):
>>
>>
>>   def need_pkt_has_multi_cof(tag):
>> -    return "A_COF" in attribdict[tag]
>> -
>> +    return (
>> +        "A_JUMP" in attribdict[tag]
>> +        or "A_CALL" in attribdict[tag]
>> +        or "J2_rte" == tag
>> +    ) and tag != "J2_hintjumpr"
> It would be better to make this decision with instruction attributes only rather than a mix of attributes and specific tags.  If needed, add another add_qemu_macro_attrib call to hex_common.calculate_attribs.
>
> Having said that, the correct tag for hintjumpr is J*4*_hintjumpr.


Good catch, thanks for finding it.  And I suppose we can change it to 
`"A_HINTJR" not in attribdict[tag]` instead.


So, now more like this:

      add_qemu_macro_attrib('fREAD_SP', 'A_IMPLICIT_READS_SP')
+    add_qemu_macro_attrib('fCLEAR_RTE_EX', 'A_RTE')

      # Recurse down macros, find attributes from sub-macros
      macroValues = list(macros.values())
@@ -291,8 +292,8 @@ def need_pkt_has_multi_cof(tag):
      return (
          "A_JUMP" in attribdict[tag]
          or "A_CALL" in attribdict[tag]
-        or "J2_rte" == tag
-    ) and tag != "J2_hintjumpr"
+        or "A_RTE" in attribdict[tag]
+    ) and "A_HINTJR" not in attribdict[tag]



> Taylor
>
>

RE: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr, hintjumpr to multi_cof

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Tuesday, April 15, 2025 12:22 PM
> To: ltaylorsimpson@gmail.com; qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org; philmd@linaro.org;
> matheus.bernardino@oss.qualcomm.com; ale@rev.ng; anjo@rev.ng;
> marco.liebel@oss.qualcomm.com; alex.bennee@linaro.org;
> quic_mburton@quicinc.com; sidneym@quicinc.com
> Subject: Re: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr,
> hintjumpr to multi_cof
> 
> 
> On 4/14/2025 12:04 PM, ltaylorsimpson@gmail.com wrote:
> >
> >> -----Original Message-----
> >> From: Brian Cain <brian.cain@oss.qualcomm.com>
> >> Sent: Monday, April 7, 2025 1:27 PM
> >> To: qemu-devel@nongnu.org
> >> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> >> philmd@linaro.org; matheus.bernardino@oss.qualcomm.com;
> ale@rev.ng;
> >> anjo@rev.ng; marco.liebel@oss.qualcomm.com;
> ltaylorsimpson@gmail.com;
> >> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com
> >> Subject: [PATCH v3 3/5] target/hexagon: Add missing A_CALL attr,
> >> hintjumpr to multi_cof
> >>
> >> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> >> ---
> >>   target/hexagon/hex_common.py | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/hexagon/hex_common.py
> >> b/target/hexagon/hex_common.py index 6803908718..a2dcb0aa2e
> 100755
> >> --- a/target/hexagon/hex_common.py
> >> +++ b/target/hexagon/hex_common.py
> >> @@ -247,8 +247,11 @@ def need_next_PC(tag):
> >>
> >>
> >>   def need_pkt_has_multi_cof(tag):
> >> -    return "A_COF" in attribdict[tag]
> >> -
> >> +    return (
> >> +        "A_JUMP" in attribdict[tag]
> >> +        or "A_CALL" in attribdict[tag]
> >> +        or "J2_rte" == tag
> >> +    ) and tag != "J2_hintjumpr"
> > It would be better to make this decision with instruction attributes only
> rather than a mix of attributes and specific tags.  If needed, add another
> add_qemu_macro_attrib call to hex_common.calculate_attribs.
> >
> > Having said that, the correct tag for hintjumpr is J*4*_hintjumpr.
> 
> 
> Good catch, thanks for finding it.  And I suppose we can change it to
> `"A_HINTJR" not in attribdict[tag]` instead.
> 
> 
> So, now more like this:
> 
>       add_qemu_macro_attrib('fREAD_SP', 'A_IMPLICIT_READS_SP')
> +    add_qemu_macro_attrib('fCLEAR_RTE_EX', 'A_RTE')
> 
>       # Recurse down macros, find attributes from sub-macros
>       macroValues = list(macros.values())
> @@ -291,8 +292,8 @@ def need_pkt_has_multi_cof(tag):
>       return (
>           "A_JUMP" in attribdict[tag]
>           or "A_CALL" in attribdict[tag]
> -        or "J2_rte" == tag
> -    ) and tag != "J2_hintjumpr"
> +        or "A_RTE" in attribdict[tag]
> +    ) and "A_HINTJR" not in attribdict[tag]

Let's take a step back here.  The goal is to eliminate the pkt_has_multi_cof parameter from helpers that don't need it, right?

So, the first step is to change a check for A_COF to a check for A_JUMP or A_CALL.  Here are the opcodes where this distinction matters
    J2_endloop*                     These have fGEN_TCG overrides so there is no helper function.
    J2_pause                            Ditto
    J2_rte                                  Ditto
    J4_hintjumpr                     This is a nop in QEMU, and there is an idef-parser emit_J4_hintjumpr function that doesn't generate any TCG.
                                                 When idef-parser is off, you get a helper call, but you could easily override this with an empty fGEN_TCG.
    J2_trap[01]                        These have helper functions, so we want to return false because the helpers don't need this argument.

So the bottom line is you can add an A_TRAP attribute attached to the fTRAP macro and then this function can be
    return "A_COF" in attribdict[tag] and "A_TRAP" not in attribdict[tag]

HTH,
Taylor