| Message ID | 20240313220141.427730-4-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: set vstart_eq_zero on vector insns | expand |
On 3/13/24 12:01, 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 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(-) > > 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 ca79571ae2..cd8235ea98 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 (env->vstart >= maxsz) { > + env->vstart = 0; > + return; > + } I think you were right to be concerned about vlmax -- you need to use startb not vstart in this comparison, otherwise you've got mixed units on the comparison. > memcpy((uint8_t *)vd + H1(i), > (uint8_t *)vs2 + H1(i), > maxsz - startb); Unrelated to this patch series, this has a big-endian host error. With big-endian, the bytes to be copied may not be sequential. if (HOST_BIG_ENDIAN && i % 8 != 0) { uint32_t j = ROUND_UP(i, 8); memcpy(vd + H(j - 1), vs2 + H1(j - 1), j - i); i = j; } memcpy(vd + i, vs2 + i, maxsz - i); r~
On 3/13/24 19:36, Richard Henderson wrote: > On 3/13/24 12:01, 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 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(-) >> >> 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 ca79571ae2..cd8235ea98 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 (env->vstart >= maxsz) { >> + env->vstart = 0; >> + return; >> + } > > I think you were right to be concerned about vlmax -- you need to use startb not vstart in this comparison, otherwise you've got mixed units on the comparison. Hm, so it seems like the comparison is broken in the translation today as well ... > >> memcpy((uint8_t *)vd + H1(i), >> (uint8_t *)vs2 + H1(i), >> maxsz - startb); > > Unrelated to this patch series, this has a big-endian host error. > With big-endian, the bytes to be copied may not be sequential. We're going for v15. Might as well fix this too. At this point I'll rename the series to "riscv: vector fixes" since there's a lot of stuff going on. > > if (HOST_BIG_ENDIAN && i % 8 != 0) { > uint32_t j = ROUND_UP(i, 8); > memcpy(vd + H(j - 1), vs2 + H1(j - 1), j - i); > i = j; > } > memcpy(vd + i, vs2 + i, maxsz - i); I'll do an extra patch with it. 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 ca79571ae2..cd8235ea98 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 (env->vstart >= maxsz) { + env->vstart = 0; + return; + } + memcpy((uint8_t *)vd + H1(i), (uint8_t *)vs2 + H1(i), maxsz - startb);