| Message ID | 20240314175704.478276-5-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: vector fixes | expand |
On 3/14/24 07:56, Daniel Henrique Barboza wrote: > These insns have 2 paths: we'll either have vstart already cleared if > vstart_eq_zero or we'll do a brcond to check if vstart >= maxsz to call > the 'vmvr_v' helper. The helper will clear vstart if it executes until > the end, or if vstart >= vl. > > For starters, the check itself is wrong: we're checking vstart >= maxsz, > when in fact we should use vstart in bytes, or 'startb' like 'vmvr_v' is > calling, to do the comparison. But even after fixing the comparison we'll > still need to clear vstart in the end, which isn't happening too. > > We want to make the helpers responsible to manage vstart, including > these corner cases, precisely to avoid these situations: > > - remove the wrong vstart >= maxsz cond from the translation; > - add a 'startb >= maxsz' cond in 'vmvr_v', and clear vstart if that > happens. > > This way we're now sure that vstart is being cleared in the end of the > execution, regardless of the path taken. > > Fixes: f714361ed7 ("target/riscv: rvv-1.0: implement vstart CSR") > Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com> > --- > target/riscv/insn_trans/trans_rvv.c.inc | 3 --- > target/riscv/vector_helper.c | 5 +++++ > 2 files changed, 5 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Fri, Mar 15, 2024 at 3:58 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > These insns have 2 paths: we'll either have vstart already cleared if > vstart_eq_zero or we'll do a brcond to check if vstart >= maxsz to call > the 'vmvr_v' helper. The helper will clear vstart if it executes until > the end, or if vstart >= vl. > > For starters, the check itself is wrong: we're checking vstart >= maxsz, > when in fact we should use vstart in bytes, or 'startb' like 'vmvr_v' is > calling, to do the comparison. But even after fixing the comparison we'll > still need to clear vstart in the end, which isn't happening too. > > We want to make the helpers responsible to manage vstart, including > these corner cases, precisely to avoid these situations: > > - remove the wrong vstart >= maxsz cond from the translation; > - add a 'startb >= maxsz' cond in 'vmvr_v', and clear vstart if that > happens. > > This way we're now sure that vstart is being cleared in the end of the > execution, regardless of the path taken. > > Fixes: f714361ed7 ("target/riscv: rvv-1.0: implement vstart CSR") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/insn_trans/trans_rvv.c.inc | 3 --- > target/riscv/vector_helper.c | 5 +++++ > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc > index 8c16a9f5b3..52c26a7834 100644 > --- a/target/riscv/insn_trans/trans_rvv.c.inc > +++ b/target/riscv/insn_trans/trans_rvv.c.inc > @@ -3664,12 +3664,9 @@ static bool trans_##NAME(DisasContext *s, arg_##NAME * a) \ > vreg_ofs(s, a->rs2), maxsz, maxsz); \ > mark_vs_dirty(s); \ > } else { \ > - TCGLabel *over = gen_new_label(); \ > - tcg_gen_brcondi_tl(TCG_COND_GEU, cpu_vstart, maxsz, over); \ > tcg_gen_gvec_2_ptr(vreg_ofs(s, a->rd), vreg_ofs(s, a->rs2), \ > tcg_env, maxsz, maxsz, 0, gen_helper_vmvr_v); \ > mark_vs_dirty(s); \ > - gen_set_label(over); \ > } \ > return true; \ > } \ > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c > index 34ac4aa808..bcc553c0e2 100644 > --- a/target/riscv/vector_helper.c > +++ b/target/riscv/vector_helper.c > @@ -5075,6 +5075,11 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState *env, uint32_t desc) > uint32_t startb = env->vstart * sewb; > uint32_t i = startb; > > + if (startb >= maxsz) { > + env->vstart = 0; > + return; > + } > + > if (HOST_BIG_ENDIAN && i % 8 != 0) { > uint32_t j = ROUND_UP(i, 8); > memcpy((uint8_t *)vd + H1(j - 1), > -- > 2.44.0 > >
On 2024/3/15 1:56, Daniel Henrique Barboza wrote: > These insns have 2 paths: we'll either have vstart already cleared if > vstart_eq_zero or we'll do a brcond to check if vstart >= maxsz to call > the 'vmvr_v' helper. The helper will clear vstart if it executes until > the end, or if vstart >= vl. > > For starters, the check itself is wrong: we're checking vstart >= maxsz, > when in fact we should use vstart in bytes, or 'startb' like 'vmvr_v' is > calling, to do the comparison. But even after fixing the comparison we'll > still need to clear vstart in the end, which isn't happening too. > > We want to make the helpers responsible to manage vstart, including > these corner cases, precisely to avoid these situations: > > - remove the wrong vstart >= maxsz cond from the translation; > - add a 'startb >= maxsz' cond in 'vmvr_v', and clear vstart if that > happens. > > This way we're now sure that vstart is being cleared in the end of the > execution, regardless of the path taken. > > Fixes: f714361ed7 ("target/riscv: rvv-1.0: implement vstart CSR") > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei > --- > target/riscv/insn_trans/trans_rvv.c.inc | 3 --- > target/riscv/vector_helper.c | 5 +++++ > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc > index 8c16a9f5b3..52c26a7834 100644 > --- a/target/riscv/insn_trans/trans_rvv.c.inc > +++ b/target/riscv/insn_trans/trans_rvv.c.inc > @@ -3664,12 +3664,9 @@ static bool trans_##NAME(DisasContext *s, arg_##NAME * a) \ > vreg_ofs(s, a->rs2), maxsz, maxsz); \ > mark_vs_dirty(s); \ > } else { \ > - TCGLabel *over = gen_new_label(); \ > - tcg_gen_brcondi_tl(TCG_COND_GEU, cpu_vstart, maxsz, over); \ > tcg_gen_gvec_2_ptr(vreg_ofs(s, a->rd), vreg_ofs(s, a->rs2), \ > tcg_env, maxsz, maxsz, 0, gen_helper_vmvr_v); \ > mark_vs_dirty(s); \ > - gen_set_label(over); \ > } \ > return true; \ > } \ > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c > index 34ac4aa808..bcc553c0e2 100644 > --- a/target/riscv/vector_helper.c > +++ b/target/riscv/vector_helper.c > @@ -5075,6 +5075,11 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState *env, uint32_t desc) > uint32_t startb = env->vstart * sewb; > uint32_t i = startb; > > + if (startb >= maxsz) { > + env->vstart = 0; > + return; > + } > + > if (HOST_BIG_ENDIAN && i % 8 != 0) { > uint32_t j = ROUND_UP(i, 8); > memcpy((uint8_t *)vd + H1(j - 1),
diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc index 8c16a9f5b3..52c26a7834 100644 --- a/target/riscv/insn_trans/trans_rvv.c.inc +++ b/target/riscv/insn_trans/trans_rvv.c.inc @@ -3664,12 +3664,9 @@ static bool trans_##NAME(DisasContext *s, arg_##NAME * a) \ vreg_ofs(s, a->rs2), maxsz, maxsz); \ mark_vs_dirty(s); \ } else { \ - TCGLabel *over = gen_new_label(); \ - tcg_gen_brcondi_tl(TCG_COND_GEU, cpu_vstart, maxsz, over); \ tcg_gen_gvec_2_ptr(vreg_ofs(s, a->rd), vreg_ofs(s, a->rs2), \ tcg_env, maxsz, maxsz, 0, gen_helper_vmvr_v); \ mark_vs_dirty(s); \ - gen_set_label(over); \ } \ return true; \ } \ diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c index 34ac4aa808..bcc553c0e2 100644 --- a/target/riscv/vector_helper.c +++ b/target/riscv/vector_helper.c @@ -5075,6 +5075,11 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState *env, uint32_t desc) uint32_t startb = env->vstart * sewb; uint32_t i = startb; + if (startb >= maxsz) { + env->vstart = 0; + return; + } + if (HOST_BIG_ENDIAN && i % 8 != 0) { uint32_t j = ROUND_UP(i, 8); memcpy((uint8_t *)vd + H1(j - 1),
These insns have 2 paths: we'll either have vstart already cleared if vstart_eq_zero or we'll do a brcond to check if vstart >= maxsz to call the 'vmvr_v' helper. The helper will clear vstart if it executes until the end, or if vstart >= vl. For starters, the check itself is wrong: we're checking vstart >= maxsz, when in fact we should use vstart in bytes, or 'startb' like 'vmvr_v' is calling, to do the comparison. But even after fixing the comparison we'll still need to clear vstart in the end, which isn't happening too. We want to make the helpers responsible to manage vstart, including these corner cases, precisely to avoid these situations: - remove the wrong vstart >= maxsz cond from the translation; - add a 'startb >= maxsz' cond in 'vmvr_v', and clear vstart if that happens. This way we're now sure that vstart is being cleared in the end of the execution, regardless of the path taken. Fixes: f714361ed7 ("target/riscv: rvv-1.0: implement vstart CSR") Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/insn_trans/trans_rvv.c.inc | 3 --- target/riscv/vector_helper.c | 5 +++++ 2 files changed, 5 insertions(+), 3 deletions(-)