Re: [PATCH 2/2] target/riscv/vector_helper.c: make vext_set_tail_elems_1s() debug only

[Daniel Henrique Barboza]

On 4/27/23 22:22, Weiwei Li wrote:
> On 2023/4/28 04:57, Daniel Henrique Barboza wrote:
> > Commit 3479a814 ("target/riscv: rvv-1.0: add VMA and VTA") added vma and
> > vta fields in the vtype register, while also defining that QEMU doesn't
> > need to have a tail agnostic policy to be compliant with the RVV spec.
> > It ended up removing all tail handling code as well. Later, commit
> > 752614ca ("target/riscv: rvv: Add tail agnostic for vector load / store
> > instructions") reintroduced the tail agnostic fill for vector load/store
> > instructions only.
> >
> > This puts QEMU in a situation where some functions are 1-filling the
> > tail elements and others don't. This is still a valid implementation,
> > but the process of 1-filling the tail elements takes valuable emulation
> > time that can be used doing anything else. If the spec doesn't demand a
> > specific tail-agostic policy, a proper software wouldn't expect any
> > policy to be in place. This means that, more often than not, the work
> > we're doing by 1-filling tail elements is wasted. We would be better of
> > if vext_set_tail_elems_1s() is removed entirely from the code.
> >
> > All this said, there's still a debug value associated with it. So,
> > instead of removing it, let's gate it with cpu->cfg.debug. This way
> > software can enable this code if desirable, but for the regular case we
> > shouldn't waste time with it.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/vector_helper.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> > index 8e6c99e573..e0a292ac24 100644
> > --- a/target/riscv/vector_helper.c
> > +++ b/target/riscv/vector_helper.c
> > @@ -272,7 +272,7 @@ static void vext_set_tail_elems_1s(CPURISCVState *env, 
> > target_ulong vl,
> >       uint32_t vta = vext_vta(desc);
> >       int k;
> > -    if (vta == 0) {
> > +    if (vta == 0 || !riscv_cpu_cfg(env)->debug)  {
>
> I think this is not correct. 'debug' property is used for debug spec. And this 
> feature is controlled by another property 'rvv_ta_all_1s' .

You're right. I wasn't aware that this flag exists:


$ git grep 'rvv_ta_all_1s'
target/riscv/cpu.c:    DEFINE_PROP_BOOL("rvv_ta_all_1s", RISCVCPU, 
cfg.rvv_ta_all_1s, false),
target/riscv/cpu.h:    bool rvv_ta_all_1s;
target/riscv/translate.c:    ctx->vta = FIELD_EX32(tb_flags, TB_FLAGS, VTA) && 
cpu->cfg.rvv_ta_all_1s;
target/riscv/translate.c:    ctx->cfg_vta_all_1s = cpu->cfg.rvv_ta_all_1s;




> By the way, cfg.rvv_ta_all_1s have been ANDed intovta value. So additional 
> check on it  is also unnecessary here.


Yes. We can drop this patch then since 'vta' is already accounting for 
ta_all_1s.


Thanks,

Daniel


> Regards,
>
> Weiwei Li
>
> >           return;
> >       }