[PATCH] Hexagon: fix HVX store new

[PATCH] Hexagon: fix HVX store new

[Matheus Tavares Bernardino]

At 09a7e7db0f (Hexagon (target/hexagon) Remove uses of
op_regs_generated.h.inc, 2024-03-06), we've changed the logic of
check_new_value() to use the new pre-calculated
packet->insn[...].dest_idx instead of calculating the index on the fly
using opcode_reginfo[...]. The dest_idx index is calculated roughly like
the following:

    for reg in iset[tag]["syntax"]:
        if reg.is_written():
            dest_idx = regno
            break

Thus, we take the first register that is writtable. Before that,
however, we also used to follow an alphabetical order on the register
type: 'd', 'e', 'x', and 'y'. No longer following that makes us select
the wrong register index and the HVX store new instruction does not
update the memory like expected.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 tests/tcg/hexagon/hvx_misc.c      | 23 +++++++++++++++++++++++
 target/hexagon/gen_trans_funcs.py |  9 ++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/tests/tcg/hexagon/hvx_misc.c b/tests/tcg/hexagon/hvx_misc.c
index 1fe14b5158..90c3733da0 100644
--- a/tests/tcg/hexagon/hvx_misc.c
+++ b/tests/tcg/hexagon/hvx_misc.c
@@ -474,6 +474,27 @@ static void test_vcombine(void)
     check_output_w(__LINE__, BUFSIZE);
 }
 
+void test_store_new()
+{
+    asm volatile(
+        "r0 = #0x12345678\n"
+        "v0 = vsplat(r0)\n"
+        "r0 = #0xff00ff00\n"
+        "v1 = vsplat(r0)\n"
+        "{\n"
+        "   vdeal(v1,v0,r0)\n"
+        "   vmem(%0) = v0.new\n"
+        "}\n"
+        :
+        : "r"(&output[0])
+        : "r0", "v0", "v1", "memory"
+    );
+    for (int i = 0; i < MAX_VEC_SIZE_BYTES / 4; i++) {
+        expect[0].w[i] = 0x12345678;
+    }
+    check_output_w(__LINE__, 1);
+}
+
 int main()
 {
     init_buffers();
@@ -515,6 +536,8 @@ int main()
 
     test_vcombine();
 
+    test_store_new();
+
     puts(err ? "FAIL" : "PASS");
     return err ? 1 : 0;
 }
diff --git a/target/hexagon/gen_trans_funcs.py b/target/hexagon/gen_trans_funcs.py
index 9f86b4edbd..30f0c73e0c 100755
--- a/target/hexagon/gen_trans_funcs.py
+++ b/target/hexagon/gen_trans_funcs.py
@@ -89,6 +89,7 @@ def gen_trans_funcs(f):
 
         new_read_idx = -1
         dest_idx = -1
+        dest_idx_reg_id = None
         has_pred_dest = "false"
         for regno, (reg_type, reg_id, *_) in enumerate(regs):
             reg = hex_common.get_register(tag, reg_type, reg_id)
@@ -97,9 +98,11 @@ def gen_trans_funcs(f):
             """))
             if reg.is_read() and reg.is_new():
                 new_read_idx = regno
-            # dest_idx should be the first destination, so check for -1
-            if reg.is_written() and dest_idx == -1:
-                dest_idx = regno
+            if reg.is_written():
+                # dest_idx should be the first destination alphabetically
+                if dest_idx_reg_id is None or reg_id < dest_idx_reg_id:
+                    dest_idx = regno
+                    dest_idx_reg_id = reg_id
             if reg_type == "P" and reg.is_written() and not reg.is_read():
                 has_pred_dest = "true"
 
-- 
2.37.2

Re: [PATCH] Hexagon: fix HVX store new

[Brian Cain]

On 5/20/2024 10:53 AM, Matheus Tavares Bernardino wrote:
> At 09a7e7db0f (Hexagon (target/hexagon) Remove uses of
> op_regs_generated.h.inc, 2024-03-06), we've changed the logic of
> check_new_value() to use the new pre-calculated
> packet->insn[...].dest_idx instead of calculating the index on the fly
> using opcode_reginfo[...]. The dest_idx index is calculated roughly like
> the following:
>
>      for reg in iset[tag]["syntax"]:
>          if reg.is_written():
>              dest_idx = regno
>              break
>
> Thus, we take the first register that is writtable. Before that,
> however, we also used to follow an alphabetical order on the register
> type: 'd', 'e', 'x', and 'y'. No longer following that makes us select
> the wrong register index and the HVX store new instruction does not
> update the memory like expected.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---


Reviewed-by: Brian Cain <bcain@quicinc.com>


>   tests/tcg/hexagon/hvx_misc.c      | 23 +++++++++++++++++++++++
>   target/hexagon/gen_trans_funcs.py |  9 ++++++---
>   2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/tests/tcg/hexagon/hvx_misc.c b/tests/tcg/hexagon/hvx_misc.c
> index 1fe14b5158..90c3733da0 100644
> --- a/tests/tcg/hexagon/hvx_misc.c
> +++ b/tests/tcg/hexagon/hvx_misc.c
> @@ -474,6 +474,27 @@ static void test_vcombine(void)
>       check_output_w(__LINE__, BUFSIZE);
>   }
>   
> +void test_store_new()
> +{
> +    asm volatile(
> +        "r0 = #0x12345678\n"
> +        "v0 = vsplat(r0)\n"
> +        "r0 = #0xff00ff00\n"
> +        "v1 = vsplat(r0)\n"
> +        "{\n"
> +        "   vdeal(v1,v0,r0)\n"
> +        "   vmem(%0) = v0.new\n"
> +        "}\n"
> +        :
> +        : "r"(&output[0])
> +        : "r0", "v0", "v1", "memory"
> +    );
> +    for (int i = 0; i < MAX_VEC_SIZE_BYTES / 4; i++) {
> +        expect[0].w[i] = 0x12345678;
> +    }
> +    check_output_w(__LINE__, 1);
> +}
> +
>   int main()
>   {
>       init_buffers();
> @@ -515,6 +536,8 @@ int main()
>   
>       test_vcombine();
>   
> +    test_store_new();
> +
>       puts(err ? "FAIL" : "PASS");
>       return err ? 1 : 0;
>   }
> diff --git a/target/hexagon/gen_trans_funcs.py b/target/hexagon/gen_trans_funcs.py
> index 9f86b4edbd..30f0c73e0c 100755
> --- a/target/hexagon/gen_trans_funcs.py
> +++ b/target/hexagon/gen_trans_funcs.py
> @@ -89,6 +89,7 @@ def gen_trans_funcs(f):
>   
>           new_read_idx = -1
>           dest_idx = -1
> +        dest_idx_reg_id = None
>           has_pred_dest = "false"
>           for regno, (reg_type, reg_id, *_) in enumerate(regs):
>               reg = hex_common.get_register(tag, reg_type, reg_id)
> @@ -97,9 +98,11 @@ def gen_trans_funcs(f):
>               """))
>               if reg.is_read() and reg.is_new():
>                   new_read_idx = regno
> -            # dest_idx should be the first destination, so check for -1
> -            if reg.is_written() and dest_idx == -1:
> -                dest_idx = regno
> +            if reg.is_written():
> +                # dest_idx should be the first destination alphabetically
> +                if dest_idx_reg_id is None or reg_id < dest_idx_reg_id:
> +                    dest_idx = regno
> +                    dest_idx_reg_id = reg_id
>               if reg_type == "P" and reg.is_written() and not reg.is_read():
>                   has_pred_dest = "true"
>   

RE: [PATCH] Hexagon: fix HVX store new

[ltaylorsimpson]

> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Sent: Monday, May 20, 2024 10:53 AM
> To: qemu-devel@nongnu.org
> Cc: ltaylorsimpson@gmail.com; sidneym@quicinc.com; bcain@quicinc.com;
> richard.henderson@linaro.org; ale@rev.ng; anjo@rev.ng
> Subject: [PATCH] Hexagon: fix HVX store new
> 
> At 09a7e7db0f (Hexagon (target/hexagon) Remove uses of
> op_regs_generated.h.inc, 2024-03-06), we've changed the logic of
> check_new_value() to use the new pre-calculated
> packet->insn[...].dest_idx instead of calculating the index on the fly
> using opcode_reginfo[...]. The dest_idx index is calculated roughly like
the
> following:
> 
>     for reg in iset[tag]["syntax"]:
>         if reg.is_written():
>             dest_idx = regno
>             break
> 
> Thus, we take the first register that is writtable. Before that, however,
we
> also used to follow an alphabetical order on the register
> type: 'd', 'e', 'x', and 'y'. No longer following that makes us select the
wrong
> register index and the HVX store new instruction does not update the
> memory like expected.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>

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

Re: [PATCH] Hexagon: fix HVX store new

[Brian Cain]

On 5/20/2024 10:53 AM, Matheus Tavares Bernardino wrote:
> At 09a7e7db0f (Hexagon (target/hexagon) Remove uses of
> op_regs_generated.h.inc, 2024-03-06), we've changed the logic of
> check_new_value() to use the new pre-calculated
> packet->insn[...].dest_idx instead of calculating the index on the fly
> using opcode_reginfo[...]. The dest_idx index is calculated roughly like
> the following:
>
>      for reg in iset[tag]["syntax"]:
>          if reg.is_written():
>              dest_idx = regno
>              break
>
> Thus, we take the first register that is writtable. Before that,
> however, we also used to follow an alphabetical order on the register
> type: 'd', 'e', 'x', and 'y'. No longer following that makes us select
> the wrong register index and the HVX store new instruction does not
> update the memory like expected.
>
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---

Queued -- at https://github.com/quic/qemu/tree/hex.next


>   tests/tcg/hexagon/hvx_misc.c      | 23 +++++++++++++++++++++++
>   target/hexagon/gen_trans_funcs.py |  9 ++++++---
>   2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/tests/tcg/hexagon/hvx_misc.c b/tests/tcg/hexagon/hvx_misc.c
> index 1fe14b5158..90c3733da0 100644
> --- a/tests/tcg/hexagon/hvx_misc.c
> +++ b/tests/tcg/hexagon/hvx_misc.c
> @@ -474,6 +474,27 @@ static void test_vcombine(void)
>       check_output_w(__LINE__, BUFSIZE);
>   }
>   
> +void test_store_new()
> +{
> +    asm volatile(
> +        "r0 = #0x12345678\n"
> +        "v0 = vsplat(r0)\n"
> +        "r0 = #0xff00ff00\n"
> +        "v1 = vsplat(r0)\n"
> +        "{\n"
> +        "   vdeal(v1,v0,r0)\n"
> +        "   vmem(%0) = v0.new\n"
> +        "}\n"
> +        :
> +        : "r"(&output[0])
> +        : "r0", "v0", "v1", "memory"
> +    );
> +    for (int i = 0; i < MAX_VEC_SIZE_BYTES / 4; i++) {
> +        expect[0].w[i] = 0x12345678;
> +    }
> +    check_output_w(__LINE__, 1);
> +}
> +
>   int main()
>   {
>       init_buffers();
> @@ -515,6 +536,8 @@ int main()
>   
>       test_vcombine();
>   
> +    test_store_new();
> +
>       puts(err ? "FAIL" : "PASS");
>       return err ? 1 : 0;
>   }
> diff --git a/target/hexagon/gen_trans_funcs.py b/target/hexagon/gen_trans_funcs.py
> index 9f86b4edbd..30f0c73e0c 100755
> --- a/target/hexagon/gen_trans_funcs.py
> +++ b/target/hexagon/gen_trans_funcs.py
> @@ -89,6 +89,7 @@ def gen_trans_funcs(f):
>   
>           new_read_idx = -1
>           dest_idx = -1
> +        dest_idx_reg_id = None
>           has_pred_dest = "false"
>           for regno, (reg_type, reg_id, *_) in enumerate(regs):
>               reg = hex_common.get_register(tag, reg_type, reg_id)
> @@ -97,9 +98,11 @@ def gen_trans_funcs(f):
>               """))
>               if reg.is_read() and reg.is_new():
>                   new_read_idx = regno
> -            # dest_idx should be the first destination, so check for -1
> -            if reg.is_written() and dest_idx == -1:
> -                dest_idx = regno
> +            if reg.is_written():
> +                # dest_idx should be the first destination alphabetically
> +                if dest_idx_reg_id is None or reg_id < dest_idx_reg_id:
> +                    dest_idx = regno
> +                    dest_idx_reg_id = reg_id
>               if reg_type == "P" and reg.is_written() and not reg.is_read():
>                   has_pred_dest = "true"
>