| Message ID | 20240206030527.169147-3-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: assorted mte fixes | expand |
On Tue, 6 Feb 2024 at 03:06, Richard Henderson <richard.henderson@linaro.org> wrote: > > The field is encoded as [0-3], which is convenient for > indexing our array of function pointers, but the true > value is [1-4]. Adjust before calling do_mem_zpa. > > Add an assert, and move the comment re passing ZT to > the helper back next to the relevant code. > > Cc: qemu-stable@nongnu.org > Fixes: 206adacfb8d ("target/arm: Add mte helpers for sve scalar + int loads") > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/tcg/translate-sve.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/target/arm/tcg/translate-sve.c b/target/arm/tcg/translate-sve.c > index 296e7d1ce2..f50a426c98 100644 > --- a/target/arm/tcg/translate-sve.c > +++ b/target/arm/tcg/translate-sve.c > @@ -4445,11 +4445,7 @@ static void do_mem_zpa(DisasContext *s, int zt, int pg, TCGv_i64 addr, > TCGv_ptr t_pg; > int desc = 0; > > - /* > - * For e.g. LD4, there are not enough arguments to pass all 4 > - * registers as pointers, so encode the regno into the data field. > - * For consistency, do this even for LD1. > - */ > + assert(mte_n >= 1 && mte_n <= 4); > if (s->mte_active[0]) { > int msz = dtype_msz(dtype); > > @@ -4463,6 +4459,11 @@ static void do_mem_zpa(DisasContext *s, int zt, int pg, TCGv_i64 addr, > addr = clean_data_tbi(s, addr); > } > > + /* > + * For e.g. LD4, there are not enough arguments to pass all 4 > + * registers as pointers, so encode the regno into the data field. > + * For consistency, do this even for LD1. > + */ > desc = simd_desc(vsz, vsz, zt | desc); > t_pg = tcg_temp_new_ptr(); > > @@ -4600,7 +4601,7 @@ static void do_ld_zpa(DisasContext *s, int zt, int pg, > * accessible via the instruction encoding. > */ > assert(fn != NULL); > - do_mem_zpa(s, zt, pg, addr, dtype, nreg, false, fn); > + do_mem_zpa(s, zt, pg, addr, dtype, nreg + 1, false, fn); > } > > static bool trans_LD_zprr(DisasContext *s, arg_rprr_load *a) What about do_st_zpa() ? It's not obvious what the 'nreg' encoding is in the a->nreg field in arg_rprr_store, but it's definitely confusing that do_st_zpa() calls do_mem_zpa() passing "nreg" whereas do_ld_zpa() now passes it "nreg + 1". Can we make it so the handling in these two functions lines up? thanks -- PMM
On 2/7/24 00:46, Peter Maydell wrote: >> @@ -4600,7 +4601,7 @@ static void do_ld_zpa(DisasContext *s, int zt, int pg, >> * accessible via the instruction encoding. >> */ >> assert(fn != NULL); >> - do_mem_zpa(s, zt, pg, addr, dtype, nreg, false, fn); >> + do_mem_zpa(s, zt, pg, addr, dtype, nreg + 1, false, fn); >> } >> >> static bool trans_LD_zprr(DisasContext *s, arg_rprr_load *a) > > What about do_st_zpa() ? It's not obvious what the 'nreg' > encoding is in the a->nreg field in arg_rprr_store, but > it's definitely confusing that do_st_zpa() calls > do_mem_zpa() passing "nreg" whereas do_ld_zpa() now > passes it "nreg + 1". Can we make it so the handling > in these two functions lines up? Yes, I think there may be a bug in store as well. Comparing the two is complicated by the cut outs for LDFF1, LDNF1, LD1R and PRF. r~
diff --git a/target/arm/tcg/translate-sve.c b/target/arm/tcg/translate-sve.c index 296e7d1ce2..f50a426c98 100644 --- a/target/arm/tcg/translate-sve.c +++ b/target/arm/tcg/translate-sve.c @@ -4445,11 +4445,7 @@ static void do_mem_zpa(DisasContext *s, int zt, int pg, TCGv_i64 addr, TCGv_ptr t_pg; int desc = 0; - /* - * For e.g. LD4, there are not enough arguments to pass all 4 - * registers as pointers, so encode the regno into the data field. - * For consistency, do this even for LD1. - */ + assert(mte_n >= 1 && mte_n <= 4); if (s->mte_active[0]) { int msz = dtype_msz(dtype); @@ -4463,6 +4459,11 @@ static void do_mem_zpa(DisasContext *s, int zt, int pg, TCGv_i64 addr, addr = clean_data_tbi(s, addr); } + /* + * For e.g. LD4, there are not enough arguments to pass all 4 + * registers as pointers, so encode the regno into the data field. + * For consistency, do this even for LD1. + */ desc = simd_desc(vsz, vsz, zt | desc); t_pg = tcg_temp_new_ptr(); @@ -4600,7 +4601,7 @@ static void do_ld_zpa(DisasContext *s, int zt, int pg, * accessible via the instruction encoding. */ assert(fn != NULL); - do_mem_zpa(s, zt, pg, addr, dtype, nreg, false, fn); + do_mem_zpa(s, zt, pg, addr, dtype, nreg + 1, false, fn); } static bool trans_LD_zprr(DisasContext *s, arg_rprr_load *a)
The field is encoded as [0-3], which is convenient for indexing our array of function pointers, but the true value is [1-4]. Adjust before calling do_mem_zpa. Add an assert, and move the comment re passing ZT to the helper back next to the relevant code. Cc: qemu-stable@nongnu.org Fixes: 206adacfb8d ("target/arm: Add mte helpers for sve scalar + int loads") Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/tcg/translate-sve.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)