Re: [PATCH v3 1/5] target/riscv: add vector unit stride load and store instructions

[LIU Zhiwei]

Hi, Richard

Thanks for comments.

On 2020/2/12 14:38, Richard Henderson wrote:
> On 2/9/20 11:42 PM, LIU Zhiwei wrote:
> > +/*
> > + * As simd_desc supports at most 256 bytes, and in this implementation,
> > + * the max vector group length is 2048 bytes. So split it into two parts.
> > + *
> > + * The first part is floor(maxsz, 64), encoded in maxsz of simd_desc.
> > + * The second part is (maxsz % 64) >> 3, encoded in data of simd_desc.
> > + */
> > +static uint32_t maxsz_part1(uint32_t maxsz)
> > +{
> > +    return ((maxsz & ~(0x3f)) >> 3) + 0x8; /* add offset 8 to avoid return 0 */
> > +}
> > +
> > +static uint32_t maxsz_part2(uint32_t maxsz)
> > +{
> > +    return (maxsz & 0x3f) >> 3;
> > +}
> I would much rather adjust simd_desc to support 2048 bytes.
>
> I've just posted a patch set that removes an assert in target/arm that would
> trigger if SIMD_DATA_SHIFT was increased to make room for a larger oprsz.
Do you mean "assert(maxsz % 8 == 0 && maxsz <= (8 << SIMD_MAXSZ_BITS));" 
in tcg-op-gvec.c?
If it is removed, I can pass 2048 bytes by set maxsz == 256.
> Or, since we're not going through tcg_gen_gvec_* for ldst, don't bother with
> simd_desc at all, and just pass vlen, unencoded.
 Vlen is not enough,  lmul is also needed in helpers.
> > +/* define check conditions data structure */
> > +struct vext_check_ctx {
> > +
> > +    struct vext_reg {
> > +        uint8_t reg;
> > +        bool widen;
> > +        bool need_check;
> > +    } check_reg[6];
> > +
> > +    struct vext_overlap_mask {
> > +        uint8_t reg;
> > +        uint8_t vm;
> > +        bool need_check;
> > +    } check_overlap_mask;
> > +
> > +    struct vext_nf {
> > +        uint8_t nf;
> > +        bool need_check;
> > +    } check_nf;
> > +    target_ulong check_misa;
> > +
> > +} vchkctx;
> You cannot use a global variable.  The data must be thread-safe.
>
> If we're going to do the checks this way, with a structure, it needs to be on
> the stack or within DisasContext.
> > +#define GEN_VEXT_LD_US_TRANS(NAME, DO_OP, SEQ)                            \
> > +static bool trans_##NAME(DisasContext *s, arg_r2nfvm* a)                  \
> > +{                                                                         \
> > +    vchkctx.check_misa = RVV;                                             \
> > +    vchkctx.check_overlap_mask.need_check = true;                         \
> > +    vchkctx.check_overlap_mask.reg = a->rd;                               \
> > +    vchkctx.check_overlap_mask.vm = a->vm;                                \
> > +    vchkctx.check_reg[0].need_check = true;                               \
> > +    vchkctx.check_reg[0].reg = a->rd;                                     \
> > +    vchkctx.check_reg[0].widen = false;                                   \
> > +    vchkctx.check_nf.need_check = true;                                   \
> > +    vchkctx.check_nf.nf = a->nf;                                          \
> > +                                                                          \
> > +    if (!vext_check(s)) {                                                 \
> > +        return false;                                                     \
> > +    }                                                                     \
> > +    return DO_OP(s, a, SEQ);                                              \
> > +}
> I don't see the improvement from a pointer.  Something like
>
>      if (vext_check_isa_ill(s) &&
>          vext_check_overlap(s, a->rd, a->rm) &&
>          vext_check_reg(s, a->rd, false) &&
>          vext_check_nf(s, a->nf)) {
>          return DO_OP(s, a, SEQ);
>      }
>      return false;
>
> seems just as clear without the extra data.
I am not quite sure which is clearer. In my opinion, setting datas is 
more easy than call different intefaces.
> > +#ifdef CONFIG_USER_ONLY
> > +#define MO_SB 0
> > +#define MO_LESW 0
> > +#define MO_LESL 0
> > +#define MO_LEQ 0
> > +#define MO_UB 0
> > +#define MO_LEUW 0
> > +#define MO_LEUL 0
> > +#endif
> What is this for?  We already define these unconditionally.
Yes. I miss a head file "exec/memop.h". When I compile in user mode,  
some make errors appear.
I will remove these codes next patch.
> > +static inline int vext_elem_mask(void *v0, int mlen, int index)
> > +{
> > +    int idx = (index * mlen) / 8;
> > +    int pos = (index * mlen) % 8;
> > +
> > +    return (*((uint8_t *)v0 + idx) >> pos) & 0x1;
> > +}
> This is a little-endian indexing of the mask.  Just above we talk about using a
> host-endian ordering of uint64_t.
>
> Thus this must be based on uint64_t instead of uint8_t.
>
> > +/*
> > + * This function checks watchpoint before really load operation.
> > + *
> > + * In softmmu mode, the TLB API probe_access is enough for watchpoint check.
> > + * In user mode, there is no watchpoint support now.
> > + *
> > + * It will triggle an exception if there is no mapping in TLB
> > + * and page table walk can't fill the TLB entry. Then the guest
> > + * software can return here after process the exception or never return.
> > + */
> > +static void probe_read_access(CPURISCVState *env, target_ulong addr,
> > +        target_ulong len, uintptr_t ra)
> > +{
> > +    while (len) {
> > +        const target_ulong pagelen = -(addr | TARGET_PAGE_MASK);
> > +        const target_ulong curlen = MIN(pagelen, len);
> > +
> > +        probe_read(env, addr, curlen, cpu_mmu_index(env, false), ra);
> The return value here is non-null when we can read directly from host memory.
> It would be a shame to throw that work away.
Yes. These host addresses can be useful. I just ignore them, because it 
will have to
add some local variables to use them. And cpu_*_mmuidx_ra will just 
search tlb table by tlb_hit.
I am not quite sure if I should keep host address in an array.

Do you think it is necessary?
> > +/* data structure and common functions for load and store */
> > +typedef void vext_ld_elem_fn(CPURISCVState *env, target_ulong addr,
> > +        uint32_t idx, void *vd, uintptr_t retaddr);
> > +typedef void vext_st_elem_fn(CPURISCVState *env, target_ulong addr,
> > +        uint32_t idx, void *vd, uintptr_t retaddr);
> > +typedef target_ulong vext_get_index_addr(target_ulong base,
> > +        uint32_t idx, void *vs2);
> > +typedef void vext_ld_clear_elem(void *vd, uint32_t idx,
> > +        uint32_t cnt, uint32_t tot);
> > +
> > +struct vext_ldst_ctx {
> > +    struct vext_common_ctx vcc;
> > +    uint32_t nf;
> > +    target_ulong base;
> > +    target_ulong stride;
> > +    int mmuidx;
> > +
> > +    vext_ld_elem_fn *ld_elem;
> > +    vext_st_elem_fn *st_elem;
> > +    vext_get_index_addr *get_index_addr;
> > +    vext_ld_clear_elem *clear_elem;
> > +};
> I think you should pass these elements directly, as needed, rather than putting
> them all in a struct.
>
> This would allow the main helper function to be inlined, which in turn allows
> the mini helper functions to be inlined.
The structure is to reduce main helper function code size and reduce the 
number of arguments
of mini helper functions.
I once pass these elements directly before this patch. It's more 
confused as so many scatted
variables and arguments.

I'm not quite sure about the efficiency improvements. If you are sure 
about that, could you
explain more details about how to achieve it.

Best Regards,
Zhiwei

> r~