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.
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.
> +/* 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.
> +#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.
> +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.
> +/* 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.
r~
