Re: [PATCH] Hexagon: fix HVX store new
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 --- 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"([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
> -Original Message- > From: Matheus Tavares Bernardino > Sent: Monday, May 20, 2024 10:53 AM > To: qemu-devel@nongnu.org > Cc: ltaylorsimp...@gmail.com; sidn...@quicinc.com; bc...@quicinc.com; > richard.hender...@linaro.org; a...@rev.ng; a...@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 Reviewed-by: Taylor Simpson
Re: [PATCH] Hexagon: fix HVX store new
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 --- Reviewed-by: Brian Cain 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"([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"