Re: [PATCH v5 05/60] target/riscv: add vector stride load and store instructions

[Alistair Francis]

On Fri, Mar 13, 2020 at 3:17 PM LIU Zhiwei <address@hidden> wrote:
>
>
>
> On 2020/3/14 6:05, Alistair Francis wrote:
> > On Fri, Mar 13, 2020 at 2:32 PM LIU Zhiwei <address@hidden> wrote:
> >>
> >>
> >> On 2020/3/14 4:38, Alistair Francis wrote:
> >>> On Thu, Mar 12, 2020 at 8:09 AM LIU Zhiwei <address@hidden> wrote:
> >>>> Vector strided operations access the first memory element at the base 
> >>>> address,
> >>>> and then access subsequent elements at address increments given by the 
> >>>> byte
> >>>> offset contained in the x register specified by rs2.
> >>>>
> >>>> Vector unit-stride operations access elements stored contiguously in 
> >>>> memory
> >>>> starting from the base effective address. It can been seen as a special
> >>>> case of strided operations.
> >>>>
> >>>> Signed-off-by: LIU Zhiwei <address@hidden>
> >>>> ---
> >>>>    target/riscv/cpu.h                      |   6 +
> >>>>    target/riscv/helper.h                   | 105 ++++++
> >>>>    target/riscv/insn32.decode              |  32 ++
> >>>>    target/riscv/insn_trans/trans_rvv.inc.c | 340 ++++++++++++++++++++
> >>>>    target/riscv/translate.c                |   7 +
> >>>>    target/riscv/vector_helper.c            | 406 ++++++++++++++++++++++++
> >>>>    6 files changed, 896 insertions(+)
> >>>>
> >>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >>>> index 505d1a8515..b6ebb9b0eb 100644
> >>>> --- a/target/riscv/cpu.h
> >>>> +++ b/target/riscv/cpu.h
> >>>> @@ -369,6 +369,12 @@ typedef CPURISCVState CPUArchState;
> >>>>    typedef RISCVCPU ArchCPU;
> >>>>    #include "exec/cpu-all.h"
> >>>>
> >>>> +/* share data between vector helpers and decode code */
> >>>> +FIELD(VDATA, MLEN, 0, 8)
> >>>> +FIELD(VDATA, VM, 8, 1)
> >>>> +FIELD(VDATA, LMUL, 9, 2)
> >>>> +FIELD(VDATA, NF, 11, 4)
> >>>> +
> >>>>    FIELD(TB_FLAGS, VL_EQ_VLMAX, 2, 1)
> >>>>    FIELD(TB_FLAGS, LMUL, 3, 2)
> >>>>    FIELD(TB_FLAGS, SEW, 5, 3)
> >>>> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> >>>> index 3c28c7e407..87dfa90609 100644
> >>>> --- a/target/riscv/helper.h
> >>>> +++ b/target/riscv/helper.h
> >>>> @@ -78,3 +78,108 @@ DEF_HELPER_1(tlb_flush, void, env)
> >>>>    #endif
> >>>>    /* Vector functions */
> >>>>    DEF_HELPER_3(vsetvl, tl, env, tl, tl)
> >>>> +DEF_HELPER_5(vlb_v_b, void, ptr, ptr, tl, env, i32)
> >>>> +DEF_HELPER_5(vlb_v_b_mask, void, ptr, ptr, tl, env, i32)
> >>> Do you mind explaining why we have *_mask versions? I'm struggling to
> >>> understand this.
> >> When an instruction with a mask, it will only operate the active
> >> elements in vector.
> >> Whether an element is active or inactive is predicated by a mask
> >> register v0.
> >>
> >> Without mask, it will operate every element in vector in the body.
> > Doesn't the mask always apply though? Why do we need an extra helper?
> Yes, mask is always applied.
>
> As you can see,  an extra helper is  very special for unit stride mode.
> Other
> instructions do not have the extra helpers.
>
> That's because a more efficient implementation is possible for unit stride
> load/store with vm==1(always unmasked).
>
> It will operate a contiguous memory block, so I can probe the memory access
> and clean the tail elements more efficient.

Ah ok. I think I get what you are saying. I think this is all ok then.
I'll review the next version (after you have split it).

Alistair

>
> Zhiwei