[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 08/20] target-mips: add msa_helper.c
From: |
Leon Alrae |
Subject: |
Re: [Qemu-devel] [PATCH 08/20] target-mips: add msa_helper.c |
Date: |
Fri, 10 Oct 2014 10:27:07 +0100 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
On 14/07/2014 10:55, Yongbok Kim wrote:
> add msa_helper.c
>
> Signed-off-by: Yongbok Kim <address@hidden>
> ---
> 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);
> + }
> +}
>
- Re: [Qemu-devel] [PATCH 08/20] target-mips: add msa_helper.c,
Leon Alrae <=