Re: [PATCH v4 05/48] target/loongarch: Implement xvadd/xvsub

[Richard Henderson <richard.henderson@linaro.org>]

On 8/30/23 01:48, Song Gao wrote:
> +#ifndef CONFIG_USER_ONLY
> + #define CHECK_VEC do { \
> +     if ((ctx->vl == LSX_LEN) && \
> +         (ctx->base.tb->flags & HW_FLAGS_EUEN_SXE) == 0) { \
> +         generate_exception(ctx, EXCCODE_SXD); \
> +         return true; \
> +     } \
> +     if ((ctx->vl == LASX_LEN) && \
> +         (ctx->base.tb->flags & HW_FLAGS_EUEN_ASXE) == 0) { \
> +         generate_exception(ctx, EXCCODE_ASXD); \
> +         return true; \
> +     } \
> + } while (0)
> +#else
> + #define CHECK_VEC
> +#endif /*!CONFIG_USER_ONLY */

I think this is wrong.  The check would seem to be determined by the instruction (oprsz) 
rather than a fixed configuration of the cpu (vl).

You're also replacing

> -#ifndef CONFIG_USER_ONLY
> -#define CHECK_ASXE do { \
> -    if ((ctx->base.tb->flags & HW_FLAGS_EUEN_ASXE) == 0) { \
> -        generate_exception(ctx, EXCCODE_ASXD); \
> -        return true; \
> -    } \
> -} while (0)
> -#else
> -#define CHECK_ASXE
> -#endif

this, the correct test, which you just added in patch 3.


> +TRANS(xvadd_b, LASX, gvec_vvv, 32, MO_8, tcg_gen_gvec_add)
> +TRANS(xvadd_h, LASX, gvec_vvv, 32, MO_16, tcg_gen_gvec_add)
> +TRANS(xvadd_w, LASX, gvec_vvv, 32, MO_32, tcg_gen_gvec_add)
> +TRANS(xvadd_d, LASX, gvec_vvv, 32, MO_64, tcg_gen_gvec_add)

The size of the changes required to add oprsz to gen_vvv would seem to be an poor choice. 
If you do go that way, all of the LSX changes would need to be a separate patch.

Perhaps better as

static bool gvec_vvv_vl(DisasContext *ctx, arg_vvv *a, uint32_t oprsz, MemOp mop,
                         void (*func)(unsigned, uint32_t, uint32_t,
                                      uint32_t, uint32_t, uint32_t))
{
     uint32_t vd_ofs = vec_full_offset(a->vd);
     uint32_t vj_ofs = vec_full_offset(a->vj);
     uint32_t vk_ofs = vec_full_offset(a->vk);

     func(mop, vd_ofs, vj_ofs, vk_ofs, oprsz, ctx->vl / 8);
     return true;
}

static bool gvec_vvv(DisasContext *ctx, arg_vvv *a, MemOp mop,
                      void (*func)(unsigned, uint32_t, uint32_t,
                                   uint32_t, uint32_t, uint32_t))
{
     CHECK_SXE;
     return gvec_vvv_vl(ctx, a, 16, mop, func);
}

static bool gvec_xxx(DisasContext *ctx, arg_vvv *a, MemOp mop,
                      void (*func)(unsigned, uint32_t, uint32_t,
                                   uint32_t, uint32_t, uint32_t))
{
     CHECK_ASXE;
     return gvec_vvv_vl(ctx, a, 32, mop, func);
}

so that you don't have to replicate "16" or "32" across each instruction.


> +#define XVADDSUB_Q(NAME)                                         \
> +static bool trans_xv## NAME ##_q(DisasContext *ctx, arg_vvv * a) \
> +{                                                                \
> +    TCGv_i64 rh, rl, ah, al, bh, bl;                             \
> +    int i;                                                       \
> +                                                                 \
> +    if (!avail_LASX(ctx)) {                                      \
> +        return false;                                            \
> +    }                                                            \
> +                                                                 \
> +    CHECK_VEC;                                                   \
> +                                                                 \
> +    rh = tcg_temp_new_i64();                                     \
> +    rl = tcg_temp_new_i64();                                     \
> +    ah = tcg_temp_new_i64();                                     \
> +    al = tcg_temp_new_i64();                                     \
> +    bh = tcg_temp_new_i64();                                     \
> +    bl = tcg_temp_new_i64();                                     \
> +                                                                 \
> +    for (i = 0; i < 2; i++) {                                    \
> +        get_vreg64(ah, a->vj, 1 + i * 2);                        \
> +        get_vreg64(al, a->vj, 0 + i * 2);                        \
> +        get_vreg64(bh, a->vk, 1 + i * 2);                        \
> +        get_vreg64(bl, a->vk, 0 + i * 2);                        \
> +                                                                 \
> +        tcg_gen_## NAME ##2_i64(rl, rh, al, ah, bl, bh);         \
> +                                                                 \
> +        set_vreg64(rh, a->vd, 1 + i * 2);                        \
> +        set_vreg64(rl, a->vd, 0 + i * 2);                        \
> +   }                                                             \
> +                                                                 \
> +    return true;                                                 \
> +}

This should be a function, not a macro, passing in tcg_gen_{add,sub}2_i64.

> +
> +XVADDSUB_Q(add)
> +XVADDSUB_Q(sub)

Which lets these be normal TRANS expansions.



r~