| Message ID | 20240313193059.405329-5-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: set vstart_eq_zero on vector insns | expand |
On 3/13/24 09:30, 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. > > However, if vstart >= maxsz, the helper will be skipped, and vstart > won't be cleared since the helper is being responsible from doing it. > > We want to make the helpers responsible to manage vstart, including > these corner cases, precisely to avoid these situations. Move the vstart >> = maxsz cond to the helper, and be sure to clear vstart if that > happens. This way we're now 100% sure that vstart is being clearer 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(-) > > 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 b4360dbd52..7260a5972b 100644 > --- a/target/riscv/vector_helper.c > +++ b/target/riscv/vector_helper.c > @@ -5163,6 +5163,11 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState *env, uint32_t desc) > > VSTART_CHECK_EARLY_EXIT(env); > > + if (env->vstart >= maxsz) { > + env->vstart = 0; > + return; > + } Did you need the VSTART_CHECK_EARLY_EXIT here then? It certainly seems like the vstart >= vl check is redundant... r~
On 3/13/24 18:16, Richard Henderson wrote: > On 3/13/24 09:30, 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. >> >> However, if vstart >= maxsz, the helper will be skipped, and vstart >> won't be cleared since the helper is being responsible from doing it. >> >> We want to make the helpers responsible to manage vstart, including >> these corner cases, precisely to avoid these situations. Move the vstart >>> = maxsz cond to the helper, and be sure to clear vstart if that >> happens. This way we're now 100% sure that vstart is being clearer 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(-) >> >> 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 b4360dbd52..7260a5972b 100644 >> --- a/target/riscv/vector_helper.c >> +++ b/target/riscv/vector_helper.c >> @@ -5163,6 +5163,11 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState *env, uint32_t desc) >> VSTART_CHECK_EARLY_EXIT(env); >> + if (env->vstart >= maxsz) { >> + env->vstart = 0; >> + return; >> + } > > Did you need the VSTART_CHECK_EARLY_EXIT here then? > It certainly seems like the vstart >= vl check is redundant... Hmm right. I thought about maxsz being calculated via vlmax, and vlmax not necessarily being == vl ... but vlmax will be always >= vl. So the vstart >= vl check s supersed by vstart >= maxsz. I'll re-send. Thanks, Daniel > > > r~
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 b4360dbd52..7260a5972b 100644 --- a/target/riscv/vector_helper.c +++ b/target/riscv/vector_helper.c @@ -5163,6 +5163,11 @@ void HELPER(vmvr_v)(void *vd, void *vs2, CPURISCVState *env, uint32_t desc) VSTART_CHECK_EARLY_EXIT(env); + if (env->vstart >= maxsz) { + env->vstart = 0; + return; + } + memcpy((uint8_t *)vd + H1(i), (uint8_t *)vs2 + H1(i), maxsz - startb);