Re: [Qemu-devel] [PATCH 08/20] target-mips: add msa_helper.c

[Leon Alrae <leon.alrae@imgtec.com>]

On 14/07/2014 10:55, Yongbok Kim wrote:
> add msa_helper.c
> 
> Signed-off-by: Yongbok Kim <yongbok.kim@imgtec.com>
> ---
>  target-mips/Makefile.objs |    2 +-
>  target-mips/msa_helper.c  |  196 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 197 insertions(+), 1 deletions(-)
>  create mode 100644 target-mips/msa_helper.c
> 
> diff --git a/target-mips/Makefile.objs b/target-mips/Makefile.objs
> index 716244f..108fd9b 100644
> --- a/target-mips/Makefile.objs
> +++ b/target-mips/Makefile.objs
> @@ -1,4 +1,4 @@
>  obj-y += translate.o dsp_helper.o op_helper.o lmi_helper.o helper.o cpu.o
> -obj-y += gdbstub.o
> +obj-y += gdbstub.o msa_helper.o
>  obj-$(CONFIG_SOFTMMU) += machine.o
>  obj-$(CONFIG_KVM) += kvm.o
> diff --git a/target-mips/msa_helper.c b/target-mips/msa_helper.c
> new file mode 100644
> index 0000000..5afc9ae
> --- /dev/null
> +++ b/target-mips/msa_helper.c
> @@ -0,0 +1,196 @@
> +/*
> + * MIPS SIMD Architecture Module Instruction emulation helpers for QEMU.
> + *
> + * Copyright (c) 2014 Imagination Technologies
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "cpu.h"
> +#include "exec/helper-proto.h"
> +
> +#define DF_BYTE   0
> +#define DF_HALF   1
> +#define DF_WORD   2
> +#define DF_DOUBLE 3

Enum probably would be nicer as we have a few related constants.

> +
> +static void msa_check_index(CPUMIPSState *env,
> +        uint32_t df, uint32_t n) {
> +    switch (df) {
> +    case DF_BYTE: /* b */
> +        if (n > MSA_WRLEN / 8 - 1) {
> +            helper_raise_exception(env, EXCP_RI);
> +        }
> +        break;
> +    case DF_HALF: /* h */
> +        if (n > MSA_WRLEN / 16 - 1) {
> +            helper_raise_exception(env, EXCP_RI);
> +        }
> +        break;
> +    case DF_WORD: /* w */
> +        if (n > MSA_WRLEN / 32 - 1) {
> +            helper_raise_exception(env, EXCP_RI);
> +        }
> +        break;
> +    case DF_DOUBLE: /* d */
> +        if (n > MSA_WRLEN / 64 - 1) {
> +            helper_raise_exception(env, EXCP_RI);
> +        }
> +        break;
> +    default:
> +        /* shouldn't get here */
> +        assert(0);
> +    }
> +}

I cannot find any place where msa_check_index would be useful. What I
can see however, is that in some msa instructions this is called 3 times
for each vector element. Please remove it.

> +
> +/* Data format min and max values */
> +#define DF_BITS(df) (1 << ((df) + 3))
> +
> +#define DF_MAX_INT(df)  (int64_t)((1LL << (DF_BITS(df) - 1)) - 1)
> +#define M_MAX_INT(m)    (int64_t)((1LL << ((m)         - 1)) - 1)
> +
> +#define DF_MIN_INT(df)  (int64_t)(-(1LL << (DF_BITS(df) - 1)))
> +#define M_MIN_INT(m)    (int64_t)(-(1LL << ((m)         - 1)))
> +
> +#define DF_MAX_UINT(df) (uint64_t)(-1ULL >> (64 - DF_BITS(df)))
> +#define M_MAX_UINT(m)   (uint64_t)(-1ULL >> (64 - (m)))
> +
> +/* Data format bit position and unsigned values */
> +#define BIT_POSITION(x, df) ((uint64_t)(x) % DF_BITS(df))
> +
> +#define UNSIGNED(x, df) ((x) & DF_MAX_UINT(df))
> +#define SIGNED(x, df)                                                   \
> +    ((((int64_t)x) << (64 - DF_BITS(df))) >> (64 - DF_BITS(df)))
> +
> +/* Element-by-element access macros */
> +#define DF_ELEMENTS(df, wrlen) (wrlen / DF_BITS(df))

wrlen as an input argument is not needed as you are always refering to
vector register size, i.e. MSA_WRLEN.

> +
> +#define  B(pwr, i) (((wr_t *)pwr)->b[i])
> +#define BR(pwr, i) (((wr_t *)pwr)->b[i])
> +#define BL(pwr, i) (((wr_t *)pwr)->b[i + MSA_WRLEN/16])

Replace void* with wr_t* in helpers instead of casting the type here.

> +
> +#define ALL_B_ELEMENTS(i, wrlen)                \
> +    do {                                        \
> +        uint32_t i;                             \
> +        for (i = wrlen / 8; i--;)
> +
> +#define  H(pwr, i) (((wr_t *)pwr)->h[i])
> +#define HR(pwr, i) (((wr_t *)pwr)->h[i])
> +#define HL(pwr, i) (((wr_t *)pwr)->h[i + MSA_WRLEN/32])
> +
> +#define ALL_H_ELEMENTS(i, wrlen)                \
> +    do {                                        \
> +        uint32_t i;                             \
> +        for (i = wrlen / 16; i--;)
> +
> +#define  W(pwr, i) (((wr_t *)pwr)->w[i])
> +#define WR(pwr, i) (((wr_t *)pwr)->w[i])
> +#define WL(pwr, i) (((wr_t *)pwr)->w[i + MSA_WRLEN/64])
> +
> +#define ALL_W_ELEMENTS(i, wrlen)                \
> +    do {                                        \
> +        uint32_t i;                             \
> +        for (i = wrlen / 32; i--;)
> +
> +#define  D(pwr, i) (((wr_t *)pwr)->d[i])
> +#define DR(pwr, i) (((wr_t *)pwr)->d[i])
> +#define DL(pwr, i) (((wr_t *)pwr)->d[i + MSA_WRLEN/128])
> +
> +#define ALL_D_ELEMENTS(i, wrlen)                \
> +    do {                                        \
> +        uint32_t i;                             \
> +        for (i = wrlen / 64; i--;)
> +
> +#define Q(pwr, i) (((wr_t *)pwr)->q[i])
> +#define ALL_Q_ELEMENTS(i, wrlen)                \
> +    do {                                        \
> +        uint32_t i;                             \
> +        for (i = wrlen / 128; i--;)
> +
> +#define DONE_ALL_ELEMENTS                       \
> +    } while (0)

I don't see any benefit from using ALL_*_ELEMENTS and DONE_ALL_ELEMENTS.
Woudn't it be better just to use regular for statements?

> +
> +static inline void msa_move_v(void *pwd, void *pws)
> +{
> +    ALL_D_ELEMENTS(i, MSA_WRLEN) {
> +        D(pwd, i) = D(pws, i);
> +    } DONE_ALL_ELEMENTS;
> +}
> +
> +static inline uint64_t msa_load_wr_elem_i64(CPUMIPSState *env, int32_t wreg,
> +        int32_t df, int32_t i)
> +{
> +    i %= DF_ELEMENTS(df, MSA_WRLEN);
> +    msa_check_index(env, (uint32_t)df, (uint32_t)i);
msa_check_index not needed.

> +
> +    switch (df) {
> +    case DF_BYTE: /* b */

The comment is redundant, data format is already indicated by DF_BYTE.
The same applies in other places.

> +        return (uint8_t)env->active_fpu.fpr[wreg].wr.b[i];
> +    case DF_HALF: /* h */
> +        return (uint16_t)env->active_fpu.fpr[wreg].wr.h[i];
> +    case DF_WORD: /* w */
> +        return (uint32_t)env->active_fpu.fpr[wreg].wr.w[i];
> +    case DF_DOUBLE: /* d */
> +        return (uint64_t)env->active_fpu.fpr[wreg].wr.d[i];
> +    default:
> +        /* shouldn't get here */
> +        assert(0);
> +    }
> +}
> +
> +static inline int64_t msa_load_wr_elem_s64(CPUMIPSState *env, int32_t wreg,
> +        int32_t df, int32_t i)
> +{
> +    i %= DF_ELEMENTS(df, MSA_WRLEN);
> +    msa_check_index(env, (uint32_t)df, (uint32_t)i);
> +
> +    switch (df) {
> +    case DF_BYTE: /* b */
> +        return env->active_fpu.fpr[wreg].wr.b[i];
> +    case DF_HALF: /* h */
> +        return env->active_fpu.fpr[wreg].wr.h[i];
> +    case DF_WORD: /* w */
> +        return env->active_fpu.fpr[wreg].wr.w[i];
> +    case DF_DOUBLE: /* d */
> +        return env->active_fpu.fpr[wreg].wr.d[i];
> +    default:
> +        /* shouldn't get here */
> +        assert(0);
> +    }
> +}
> +
> +static inline void msa_store_wr_elem(CPUMIPSState *env, uint64_t val,
> +        int32_t wreg, int32_t df, int32_t i)
> +{
> +    i %= DF_ELEMENTS(df, MSA_WRLEN);
> +    msa_check_index(env, (uint32_t)df, (uint32_t)i);
> +
> +    switch (df) {
> +    case DF_BYTE: /* b */
> +        env->active_fpu.fpr[wreg].wr.b[i] = (uint8_t)val;
> +        break;
> +    case DF_HALF: /* h */
> +        env->active_fpu.fpr[wreg].wr.h[i] = (uint16_t)val;
> +        break;
> +    case DF_WORD: /* w */
> +        env->active_fpu.fpr[wreg].wr.w[i] = (uint32_t)val;
> +        break;
> +    case DF_DOUBLE: /* d */
> +        env->active_fpu.fpr[wreg].wr.d[i] = (uint64_t)val;
> +        break;
> +    default:
> +        /* shouldn't get here */
> +        assert(0);
> +    }
> +}
>