I noticed that analyze_packet is marking the implicit pred reads after
marking all the writes.  However, the semantics of the instrucion and
packet are to do all the reads, then do the operation, then do all the
writes.

Here is the old code
static void analyze_packet(DisasContext *ctx)
{
    Packet *pkt = ctx->pkt;
    ctx->read_after_write = false;
    ctx->has_hvx_overlap = false;
    for (int i = 0; i < pkt->num_insns; i++) {
        Insn *insn = &pkt->insn[i];
        ctx->insn = insn;
        if (opcode_analyze[insn->opcode]) {
            opcode_analyze[insn->opcode](ctx);
        }
        mark_implicit_reg_writes(ctx);
        mark_implicit_pred_writes(ctx);
        mark_implicit_pred_reads(ctx);
    }

    ctx->need_commit = need_commit(ctx);
}

Recall that opcode_analyze[insn->opcode](ctx) will mark all the
explicit reads then all the explicit writes.

To properly handle the semantics, we'll create two new functions
    mark_implicit_reads
    mark_implicit_writes
Then we change gen_analyze_funcs.py to add a call to the former
after all the explicit reads and a call to the latter after all
the explicit_writes.

The reason this is an RFC patch is I can't find any instructions
where this distinction makes a difference in ctx->need_commit which
determines if the packet commit can be short-circuited.  However, this
could change in the future if the architecture introduces an
instruction with an implicit read of a register that is also written
(either implicit or explicit).  Then, anlayze_packet would detect
a read-after-write, and the packet would not short-circuit.  The
execution would be correct, but the performance would not be optimal.

Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
---
 target/hexagon/translate.c          | 18 +++++++++++++++---
 target/hexagon/gen_analyze_funcs.py |  4 ++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index fe7858703c..5271c4e022 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -37,6 +37,10 @@
 #include "exec/helper-info.c.inc"
 #undef  HELPER_H
 
+/* Forward declarations referenced in analyze_funcs_generated.c.inc */
+static void mark_implicit_reads(DisasContext *ctx);
+static void mark_implicit_writes(DisasContext *ctx);
+
 #include "analyze_funcs_generated.c.inc"
 
 typedef void (*AnalyzeInsn)(DisasContext *ctx);
@@ -378,6 +382,17 @@ static void mark_implicit_pred_reads(DisasContext *ctx)
     mark_implicit_pred_read(ctx, A_IMPLICIT_READS_P3, 3);
 }
 
+static void mark_implicit_reads(DisasContext *ctx)
+{
+    mark_implicit_pred_reads(ctx);
+}
+
+static void mark_implicit_writes(DisasContext *ctx)
+{
+    mark_implicit_reg_writes(ctx);
+    mark_implicit_pred_writes(ctx);
+}
+
 static void analyze_packet(DisasContext *ctx)
 {
     Packet *pkt = ctx->pkt;
@@ -389,9 +404,6 @@ static void analyze_packet(DisasContext *ctx)
         if (opcode_analyze[insn->opcode]) {
             opcode_analyze[insn->opcode](ctx);
         }
-        mark_implicit_reg_writes(ctx);
-        mark_implicit_pred_writes(ctx);
-        mark_implicit_pred_reads(ctx);
     }
 
     ctx->need_commit = need_commit(ctx);
diff --git a/target/hexagon/gen_analyze_funcs.py b/target/hexagon/gen_analyze_funcs.py
index 3ac7cc2cfe..fdefd5b4b3 100755
--- a/target/hexagon/gen_analyze_funcs.py
+++ b/target/hexagon/gen_analyze_funcs.py
@@ -67,6 +67,8 @@ def gen_analyze_func(f, tag, regs, imms):
         if reg.is_read():
             reg.analyze_read(f, regno)
 
+    f.write("    mark_implicit_reads(ctx);\n")
+
     ## Analyze the register writes
     for regno, register in enumerate(regs):
         reg_type, reg_id = register
@@ -74,6 +76,8 @@ def gen_analyze_func(f, tag, regs, imms):
         if reg.is_written():
             reg.analyze_write(f, tag, regno)
 
+    f.write("    mark_implicit_writes(ctx);\n")
+
     f.write("}\n\n")
 
 
-- 
2.43.0

On 3/24/2025 9:14 PM, Taylor Simpson wrote:
> I noticed that analyze_packet is marking the implicit pred reads after
> marking all the writes.  However, the semantics of the instrucion and
> packet are to do all the reads, then do the operation, then do all the
> writes.
>
> Here is the old code
> static void analyze_packet(DisasContext *ctx)
> {
>      Packet *pkt = ctx->pkt;
>      ctx->read_after_write = false;
>      ctx->has_hvx_overlap = false;
>      for (int i = 0; i < pkt->num_insns; i++) {
>          Insn *insn = &pkt->insn[i];
>          ctx->insn = insn;
>          if (opcode_analyze[insn->opcode]) {
>              opcode_analyze[insn->opcode](ctx);
>          }
>          mark_implicit_reg_writes(ctx);
>          mark_implicit_pred_writes(ctx);
>          mark_implicit_pred_reads(ctx);
>      }
>
>      ctx->need_commit = need_commit(ctx);
> }
>
> Recall that opcode_analyze[insn->opcode](ctx) will mark all the
> explicit reads then all the explicit writes.
>
> To properly handle the semantics, we'll create two new functions
>      mark_implicit_reads
>      mark_implicit_writes
> Then we change gen_analyze_funcs.py to add a call to the former
> after all the explicit reads and a call to the latter after all
> the explicit_writes.
>
> The reason this is an RFC patch is I can't find any instructions
> where this distinction makes a difference in ctx->need_commit which
> determines if the packet commit can be short-circuited.  However, this
> could change in the future if the architecture introduces an
> instruction with an implicit read of a register that is also written
> (either implicit or explicit).  Then, anlayze_packet would detect
> a read-after-write, and the packet would not short-circuit.  The
> execution would be correct, but the performance would not be optimal.
>
> Signed-off-by: Taylor Simpson <ltaylorsimpson@gmail.com>
> ---

I found no regressions when testing this patch downstream.  I agree that 
this reordering makes sense regardless of whether we can find a test 
that fails now.


Reviewed-by: Brian Cain <brian.cain@oss.qualcomm.com>

Tested-by: Brian Cain <brian.cain@oss.qualcomm.com>


>   target/hexagon/translate.c          | 18 +++++++++++++++---
>   target/hexagon/gen_analyze_funcs.py |  4 ++++
>   2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
> index fe7858703c..5271c4e022 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -37,6 +37,10 @@
>   #include "exec/helper-info.c.inc"
>   #undef  HELPER_H
>   
> +/* Forward declarations referenced in analyze_funcs_generated.c.inc */
> +static void mark_implicit_reads(DisasContext *ctx);
> +static void mark_implicit_writes(DisasContext *ctx);
> +
>   #include "analyze_funcs_generated.c.inc"
>   
>   typedef void (*AnalyzeInsn)(DisasContext *ctx);
> @@ -378,6 +382,17 @@ static void mark_implicit_pred_reads(DisasContext *ctx)
>       mark_implicit_pred_read(ctx, A_IMPLICIT_READS_P3, 3);
>   }
>   
> +static void mark_implicit_reads(DisasContext *ctx)
> +{
> +    mark_implicit_pred_reads(ctx);
> +}
> +
> +static void mark_implicit_writes(DisasContext *ctx)
> +{
> +    mark_implicit_reg_writes(ctx);
> +    mark_implicit_pred_writes(ctx);
> +}
> +
>   static void analyze_packet(DisasContext *ctx)
>   {
>       Packet *pkt = ctx->pkt;
> @@ -389,9 +404,6 @@ static void analyze_packet(DisasContext *ctx)
>           if (opcode_analyze[insn->opcode]) {
>               opcode_analyze[insn->opcode](ctx);
>           }
> -        mark_implicit_reg_writes(ctx);
> -        mark_implicit_pred_writes(ctx);
> -        mark_implicit_pred_reads(ctx);
>       }
>   
>       ctx->need_commit = need_commit(ctx);
> diff --git a/target/hexagon/gen_analyze_funcs.py b/target/hexagon/gen_analyze_funcs.py
> index 3ac7cc2cfe..fdefd5b4b3 100755
> --- a/target/hexagon/gen_analyze_funcs.py
> +++ b/target/hexagon/gen_analyze_funcs.py
> @@ -67,6 +67,8 @@ def gen_analyze_func(f, tag, regs, imms):
>           if reg.is_read():
>               reg.analyze_read(f, regno)
>   
> +    f.write("    mark_implicit_reads(ctx);\n")
> +
>       ## Analyze the register writes
>       for regno, register in enumerate(regs):
>           reg_type, reg_id = register
> @@ -74,6 +76,8 @@ def gen_analyze_func(f, tag, regs, imms):
>           if reg.is_written():
>               reg.analyze_write(f, tag, regno)
>   
> +    f.write("    mark_implicit_writes(ctx);\n")
> +
>       f.write("}\n\n")
>   
>