| Message ID | f548dc1c240819c724245e887f29f918441e9125.1716220379.git.quic_mathbern@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hexagon: fix HVX store new | expand |
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" >
> -----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> > --- 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" >
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"
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(-)