tests/tcg/hexagon/hvx_misc.c | 23 +++++++++++++++++++++++ target/hexagon/gen_trans_funcs.py | 9 ++++++--- 2 files changed, 29 insertions(+), 3 deletions(-)
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
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"
>
> -----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>
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"
>
© 2016 - 2024 Red Hat, Inc.