diff mbox

[v2,5/8] target-mips: use DSP unions for binary DSP operators

Message ID 1357745265-16084-6-git-send-email-aurelien@aurel32.net
State New
Headers show

Commit Message

Aurelien Jarno Jan. 9, 2013, 3:27 p.m. UTC
This allow to reduce the number of macros.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 target-mips/dsp_helper.c |  384 ++++++++++++++--------------------------------
 1 file changed, 116 insertions(+), 268 deletions(-)

Comments

Blue Swirl Jan. 9, 2013, 9:16 p.m. UTC | #1
On Wed, Jan 9, 2013 at 3:27 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> This allow to reduce the number of macros.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  target-mips/dsp_helper.c |  384 ++++++++++++++--------------------------------
>  1 file changed, 116 insertions(+), 268 deletions(-)
>
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index aed4c63..e01c8a9 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -1078,7 +1078,6 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
>          b = num & MIPSDSP_LO;               \
>      } while (0)
>
> -#define MIPSDSP_RETURN32(a)             ((target_long)(int32_t)a)
>  #define MIPSDSP_RETURN32_8(a, b, c, d)  ((target_long)(int32_t) \
>                                           (((uint32_t)a << 24) | \
>                                           (((uint32_t)b << 16) | \
> @@ -1111,119 +1110,127 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
>  #endif
>
>  /** DSP Arithmetic Sub-class insns **/
> -#define ARITH_PH(name, func)                                      \
> -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt) \
> -{                                                                 \
> -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
> -                                                                  \
> -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
> -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
> -                                                                  \
> -    temph = mipsdsp_##func(rsh, rth);                             \
> -    templ = mipsdsp_##func(rsl, rtl);                             \
> -                                                                  \
> -    return MIPSDSP_RETURN32_16(temph, templ);                     \
> -}
> -
> -#define ARITH_PH_ENV(name, func)                                  \
> -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt, \
> -                                CPUMIPSState *env)                \
> -{                                                                 \
> -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
> -                                                                  \
> -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
> -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
> -                                                                  \
> -    temph = mipsdsp_##func(rsh, rth, env);                        \
> -    templ = mipsdsp_##func(rsl, rtl, env);                        \
> -                                                                  \
> -    return MIPSDSP_RETURN32_16(temph, templ);                     \
> -}
> -
> -
> -ARITH_PH_ENV(addq, add_i16);
> -ARITH_PH_ENV(addq_s, sat_add_i16);
> -ARITH_PH_ENV(addu, add_u16);
> -ARITH_PH_ENV(addu_s, sat_add_u16);
> -
> -ARITH_PH(addqh, rshift1_add_q16);
> -ARITH_PH(addqh_r, rrshift1_add_q16);
> -
> -ARITH_PH_ENV(subq, sub_i16);
> -ARITH_PH_ENV(subq_s, sat16_sub);
> -ARITH_PH_ENV(subu, sub_u16_u16);
> -ARITH_PH_ENV(subu_s, satu16_sub_u16_u16);
> -
> -ARITH_PH(subqh, rshift1_sub_q16);
> -ARITH_PH(subqh_r, rrshift1_sub_q16);
> -
> -#undef ARITH_PH
> -#undef ARITH_PH_ENV
> +#define MIPSDSP32_BINOP(name, func, element)                               \
> +target_ulong helper_##name(target_ulong rs, target_ulong rt)               \
> +{                                                                          \
> +    DSP32Value ds, dt;                                                     \
> +    unsigned int i, n;                                                     \
> +                                                                           \
> +    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                        \
> +    ds.sw[0] = rs;                                                         \
> +    dt.sw[0] = rt;                                                         \
> +                                                                           \
> +    for (i = 0 ; i < n ; i++) {                                            \

There's an extra space before ';', please remove. Also in the other
for loops below.

> +        ds.element[i] = mipsdsp_##func(ds.element[i], dt.element[i]);      \
> +    }                                                                      \
> +                                                                           \
> +    return (target_long)ds.sw[0];                                          \
> +}
> +MIPSDSP32_BINOP(addqh_ph, rshift1_add_q16, sh);
> +MIPSDSP32_BINOP(addqh_r_ph, rrshift1_add_q16, sh);
> +MIPSDSP32_BINOP(addqh_r_w, rrshift1_add_q32, sw);
> +MIPSDSP32_BINOP(addqh_w, rshift1_add_q32, sw);
> +MIPSDSP32_BINOP(adduh_qb, rshift1_add_u8, ub);
> +MIPSDSP32_BINOP(adduh_r_qb, rrshift1_add_u8, ub);
> +MIPSDSP32_BINOP(subqh_ph, rshift1_sub_q16, sh);
> +MIPSDSP32_BINOP(subqh_r_ph, rrshift1_sub_q16, sh);
> +MIPSDSP32_BINOP(subqh_r_w, rrshift1_sub_q32, sw);
> +MIPSDSP32_BINOP(subqh_w, rshift1_sub_q32, sw);
> +#undef MIPSDSP32_BINOP
> +
> +#define MIPSDSP32_BINOP_ENV(name, func, element)                           \
> +target_ulong helper_##name(target_ulong rs, target_ulong rt,               \
> +                           CPUMIPSState *env)                              \
> +{                                                                          \
> +    DSP32Value ds, dt;                                                     \
> +    unsigned int i, n;                                                     \
> +                                                                           \
> +    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                        \
> +    ds.sw[0] = rs;                                                         \
> +    dt.sw[0] = rt;                                                         \
> +                                                                           \
> +    for (i = 0 ; i < n ; i++) {                                            \
> +        ds.element[i] = mipsdsp_##func(ds.element[i], dt.element[i], env); \
> +    }                                                                      \
> +                                                                           \
> +    return (target_long)ds.sw[0];                                          \
> +}
> +MIPSDSP32_BINOP_ENV(addq_ph, add_i16, sh)
> +MIPSDSP32_BINOP_ENV(addq_s_ph, sat_add_i16, sh)
> +MIPSDSP32_BINOP_ENV(addq_s_w, sat_add_i32, sw);
> +MIPSDSP32_BINOP_ENV(addu_ph, add_u16, sh)
> +MIPSDSP32_BINOP_ENV(addu_qb, add_u8, ub);
> +MIPSDSP32_BINOP_ENV(addu_s_ph, sat_add_u16, sh)
> +MIPSDSP32_BINOP_ENV(addu_s_qb, sat_add_u8, ub);
> +MIPSDSP32_BINOP_ENV(subq_ph, sub_i16, sh);
> +MIPSDSP32_BINOP_ENV(subq_s_ph, sat16_sub, sh);
> +MIPSDSP32_BINOP_ENV(subq_s_w, sat32_sub, sw);
> +MIPSDSP32_BINOP_ENV(subu_ph, sub_u16_u16, sh);
> +MIPSDSP32_BINOP_ENV(subu_qb, sub_u8, ub);
> +MIPSDSP32_BINOP_ENV(subu_s_ph, satu16_sub_u16_u16, sh);
> +MIPSDSP32_BINOP_ENV(subu_s_qb, satu8_sub, ub);
> +#undef MIPSDSP32_BINOP_ENV
>
>  #ifdef TARGET_MIPS64
> -#define ARITH_QH_ENV(name, func) \
> -target_ulong helper_##name##_qh(target_ulong rs, target_ulong rt, \
> -                                CPUMIPSState *env)           \
> -{                                                            \
> -    uint16_t rs3, rs2, rs1, rs0;                             \
> -    uint16_t rt3, rt2, rt1, rt0;                             \
> -    uint16_t tempD, tempC, tempB, tempA;                     \
> -                                                             \
> -    MIPSDSP_SPLIT64_16(rs, rs3, rs2, rs1, rs0);              \
> -    MIPSDSP_SPLIT64_16(rt, rt3, rt2, rt1, rt0);              \
> -                                                             \
> -    tempD = mipsdsp_##func(rs3, rt3, env);                   \
> -    tempC = mipsdsp_##func(rs2, rt2, env);                   \
> -    tempB = mipsdsp_##func(rs1, rt1, env);                   \
> -    tempA = mipsdsp_##func(rs0, rt0, env);                   \
> -                                                             \
> -    return MIPSDSP_RETURN64_16(tempD, tempC, tempB, tempA);  \
> -}
> -
> -ARITH_QH_ENV(addq, add_i16);
> -ARITH_QH_ENV(addq_s, sat_add_i16);
> -ARITH_QH_ENV(addu, add_u16);
> -ARITH_QH_ENV(addu_s, sat_add_u16);
> -
> -ARITH_QH_ENV(subq, sub_i16);
> -ARITH_QH_ENV(subq_s, sat16_sub);
> -ARITH_QH_ENV(subu, sub_u16_u16);
> -ARITH_QH_ENV(subu_s, satu16_sub_u16_u16);
> -
> -#undef ARITH_QH_ENV
> +#define MIPSDSP64_BINOP(name, func, element)                               \
> +target_ulong helper_##name(target_ulong rs, target_ulong rt)               \
> +{                                                                          \
> +    DSP64Value ds, dt;                                                     \
> +    unsigned int i, n;                                                     \
> +                                                                           \
> +    n = sizeof(DSP64Value) / sizeof(ds.element[0]);                        \
> +    ds.sl[0] = rs;                                                         \
> +    dt.sl[0] = rt;                                                         \
> +                                                                           \
> +    for (i = 0 ; i < n ; i++) {                                            \
> +        ds.element[i] = mipsdsp_##func(ds.element[i], dt.element[i]);      \
> +    }                                                                      \
> +                                                                           \
> +    return ds.sl[0];                                                       \
> +}
> +MIPSDSP64_BINOP(adduh_ob, rshift1_add_u8, ub);
> +MIPSDSP64_BINOP(adduh_r_ob, rrshift1_add_u8, ub);
> +MIPSDSP64_BINOP(subuh_ob, rshift1_sub_u8, ub);
> +MIPSDSP64_BINOP(subuh_r_ob, rrshift1_sub_u8, ub);
> +#undef MIPSDSP64_BINOP
> +
> +#define MIPSDSP64_BINOP_ENV(name, func, element)                           \
> +target_ulong helper_##name(target_ulong rs, target_ulong rt,               \
> +                           CPUMIPSState *env)                              \
> +{                                                                          \
> +    DSP64Value ds, dt;                                                     \
> +    unsigned int i, n;                                                     \
> +                                                                           \
> +    n = sizeof(DSP64Value) / sizeof(ds.element[0]);                        \
> +    ds.sl[0] = rs;                                                         \
> +    dt.sl[0] = rt;                                                         \
> +                                                                           \
> +    for (i = 0 ; i < n ; i++) {                                            \
> +        ds.element[i] = mipsdsp_##func(ds.element[i], dt.element[i], env); \
> +    }                                                                      \
> +                                                                           \
> +    return ds.sl[0];                                                       \
> +}
> +MIPSDSP64_BINOP_ENV(addq_pw, add_i32, sw);
> +MIPSDSP64_BINOP_ENV(addq_qh, add_i16, sh);
> +MIPSDSP64_BINOP_ENV(addq_s_pw, sat_add_i32, sw);
> +MIPSDSP64_BINOP_ENV(addq_s_qh, sat_add_i16, sh);
> +MIPSDSP64_BINOP_ENV(addu_ob, add_u8, uh);
> +MIPSDSP64_BINOP_ENV(addu_qh, add_u16, uh);
> +MIPSDSP64_BINOP_ENV(addu_s_ob, sat_add_u8, uh);
> +MIPSDSP64_BINOP_ENV(addu_s_qh, sat_add_u16, uh);
> +MIPSDSP64_BINOP_ENV(subq_pw, sub32, sw);
> +MIPSDSP64_BINOP_ENV(subq_qh, sub_i16, sh);
> +MIPSDSP64_BINOP_ENV(subq_s_pw, sat32_sub, sw);
> +MIPSDSP64_BINOP_ENV(subq_s_qh, sat16_sub, sh);
> +MIPSDSP64_BINOP_ENV(subu_ob, sub_u8, uh);
> +MIPSDSP64_BINOP_ENV(subu_qh, sub_u16_u16, uh);
> +MIPSDSP64_BINOP_ENV(subu_s_ob, satu8_sub, uh);
> +MIPSDSP64_BINOP_ENV(subu_s_qh, satu16_sub_u16_u16, uh);
> +#undef MIPSDSP64_BINOP_ENV
>
>  #endif
>
> -#define ARITH_W(name, func) \
> -target_ulong helper_##name##_w(target_ulong rs, target_ulong rt) \
> -{                                                                \
> -    uint32_t rd;                                                 \
> -    rd = mipsdsp_##func(rs, rt);                                 \
> -    return MIPSDSP_RETURN32(rd);                                 \
> -}
> -
> -#define ARITH_W_ENV(name, func) \
> -target_ulong helper_##name##_w(target_ulong rs, target_ulong rt, \
> -                               CPUMIPSState *env)                \
> -{                                                                \
> -    uint32_t rd;                                                 \
> -    rd = mipsdsp_##func(rs, rt, env);                            \
> -    return MIPSDSP_RETURN32(rd);                                 \
> -}
> -
> -ARITH_W_ENV(addq_s, sat_add_i32);
> -
> -ARITH_W(addqh, rshift1_add_q32);
> -ARITH_W(addqh_r, rrshift1_add_q32);
> -
> -ARITH_W_ENV(subq_s, sat32_sub);
> -
> -ARITH_W(subqh, rshift1_sub_q32);
> -ARITH_W(subqh_r, rrshift1_sub_q32);
> -
> -#undef ARITH_W
> -#undef ARITH_W_ENV
> -
>  target_ulong helper_absq_s_w(target_ulong rt, CPUMIPSState *env)
>  {
>      uint32_t rd;
> @@ -1234,164 +1241,6 @@ target_ulong helper_absq_s_w(target_ulong rt, CPUMIPSState *env)
>  }
>
>
> -#if defined(TARGET_MIPS64)
> -
> -#define ARITH_PW_ENV(name, func) \
> -target_ulong helper_##name##_pw(target_ulong rs, target_ulong rt, \
> -                                CPUMIPSState *env)                \
> -{                                                                 \
> -    uint32_t rs1, rs0;                                            \
> -    uint32_t rt1, rt0;                                            \
> -    uint32_t tempB, tempA;                                        \
> -                                                                  \
> -    MIPSDSP_SPLIT64_32(rs, rs1, rs0);                             \
> -    MIPSDSP_SPLIT64_32(rt, rt1, rt0);                             \
> -                                                                  \
> -    tempB = mipsdsp_##func(rs1, rt1, env);                        \
> -    tempA = mipsdsp_##func(rs0, rt0, env);                        \
> -                                                                  \
> -    return MIPSDSP_RETURN64_32(tempB, tempA);                     \
> -}
> -
> -ARITH_PW_ENV(addq, add_i32);
> -ARITH_PW_ENV(addq_s, sat_add_i32);
> -ARITH_PW_ENV(subq, sub32);
> -ARITH_PW_ENV(subq_s, sat32_sub);
> -
> -#undef ARITH_PW_ENV
> -
> -#endif
> -
> -#define ARITH_QB(name, func) \
> -target_ulong helper_##name##_qb(target_ulong rs, target_ulong rt) \
> -{                                                                 \
> -    uint8_t  rs0, rs1, rs2, rs3;                                  \
> -    uint8_t  rt0, rt1, rt2, rt3;                                  \
> -    uint8_t  temp0, temp1, temp2, temp3;                          \
> -                                                                  \
> -    MIPSDSP_SPLIT32_8(rs, rs3, rs2, rs1, rs0);                    \
> -    MIPSDSP_SPLIT32_8(rt, rt3, rt2, rt1, rt0);                    \
> -                                                                  \
> -    temp0 = mipsdsp_##func(rs0, rt0);                             \
> -    temp1 = mipsdsp_##func(rs1, rt1);                             \
> -    temp2 = mipsdsp_##func(rs2, rt2);                             \
> -    temp3 = mipsdsp_##func(rs3, rt3);                             \
> -                                                                  \
> -    return MIPSDSP_RETURN32_8(temp3, temp2, temp1, temp0);        \
> -}
> -
> -#define ARITH_QB_ENV(name, func) \
> -target_ulong helper_##name##_qb(target_ulong rs, target_ulong rt, \
> -                                CPUMIPSState *env)          \
> -{                                                           \
> -    uint8_t  rs0, rs1, rs2, rs3;                            \
> -    uint8_t  rt0, rt1, rt2, rt3;                            \
> -    uint8_t  temp0, temp1, temp2, temp3;                    \
> -                                                            \
> -    MIPSDSP_SPLIT32_8(rs, rs3, rs2, rs1, rs0);              \
> -    MIPSDSP_SPLIT32_8(rt, rt3, rt2, rt1, rt0);              \
> -                                                            \
> -    temp0 = mipsdsp_##func(rs0, rt0, env);                  \
> -    temp1 = mipsdsp_##func(rs1, rt1, env);                  \
> -    temp2 = mipsdsp_##func(rs2, rt2, env);                  \
> -    temp3 = mipsdsp_##func(rs3, rt3, env);                  \
> -                                                            \
> -    return MIPSDSP_RETURN32_8(temp3, temp2, temp1, temp0);  \
> -}
> -
> -ARITH_QB(adduh, rshift1_add_u8);
> -ARITH_QB(adduh_r, rrshift1_add_u8);
> -
> -ARITH_QB_ENV(addu, add_u8);
> -ARITH_QB_ENV(addu_s, sat_add_u8);
> -
> -#undef ADDU_QB
> -#undef ADDU_QB_ENV
> -
> -#if defined(TARGET_MIPS64)
> -#define ARITH_OB(name, func) \
> -target_ulong helper_##name##_ob(target_ulong rs, target_ulong rt) \
> -{                                                                 \
> -    int i;                                                        \
> -    uint8_t rs_t[8], rt_t[8];                                     \
> -    uint8_t temp[8];                                              \
> -    uint64_t result;                                              \
> -                                                                  \
> -    result = 0;                                                   \
> -                                                                  \
> -    for (i = 0; i < 8; i++) {                                     \
> -        rs_t[i] = (rs >> (8 * i)) & MIPSDSP_Q0;                   \
> -        rt_t[i] = (rt >> (8 * i)) & MIPSDSP_Q0;                   \
> -        temp[i] = mipsdsp_##func(rs_t[i], rt_t[i]);               \
> -        result |= (uint64_t)temp[i] << (8 * i);                   \
> -    }                                                             \
> -                                                                  \
> -    return result;                                                \
> -}
> -
> -#define ARITH_OB_ENV(name, func) \
> -target_ulong helper_##name##_ob(target_ulong rs, target_ulong rt, \
> -                                CPUMIPSState *env)                \
> -{                                                                 \
> -    int i;                                                        \
> -    uint8_t rs_t[8], rt_t[8];                                     \
> -    uint8_t temp[8];                                              \
> -    uint64_t result;                                              \
> -                                                                  \
> -    result = 0;                                                   \
> -                                                                  \
> -    for (i = 0; i < 8; i++) {                                     \
> -        rs_t[i] = (rs >> (8 * i)) & MIPSDSP_Q0;                   \
> -        rt_t[i] = (rt >> (8 * i)) & MIPSDSP_Q0;                   \
> -        temp[i] = mipsdsp_##func(rs_t[i], rt_t[i], env);          \
> -        result |= (uint64_t)temp[i] << (8 * i);                   \
> -    }                                                             \
> -                                                                  \
> -    return result;                                                \
> -}
> -
> -ARITH_OB_ENV(addu, add_u8);
> -ARITH_OB_ENV(addu_s, sat_add_u8);
> -
> -ARITH_OB(adduh, rshift1_add_u8);
> -ARITH_OB(adduh_r, rrshift1_add_u8);
> -
> -ARITH_OB_ENV(subu, sub_u8);
> -ARITH_OB_ENV(subu_s, satu8_sub);
> -
> -ARITH_OB(subuh, rshift1_sub_u8);
> -ARITH_OB(subuh_r, rrshift1_sub_u8);
> -
> -#undef ARITH_OB
> -#undef ARITH_OB_ENV
> -
> -#endif
> -
> -#define SUBU_QB(name, func) \
> -target_ulong helper_##name##_qb(target_ulong rs,               \
> -                                target_ulong rt,               \
> -                                CPUMIPSState *env)             \
> -{                                                              \
> -    uint8_t rs3, rs2, rs1, rs0;                                \
> -    uint8_t rt3, rt2, rt1, rt0;                                \
> -    uint8_t tempD, tempC, tempB, tempA;                        \
> -                                                               \
> -    MIPSDSP_SPLIT32_8(rs, rs3, rs2, rs1, rs0);                 \
> -    MIPSDSP_SPLIT32_8(rt, rt3, rt2, rt1, rt0);                 \
> -                                                               \
> -    tempD = mipsdsp_##func(rs3, rt3, env);                     \
> -    tempC = mipsdsp_##func(rs2, rt2, env);                     \
> -    tempB = mipsdsp_##func(rs1, rt1, env);                     \
> -    tempA = mipsdsp_##func(rs0, rt0, env);                     \
> -                                                               \
> -    return MIPSDSP_RETURN32_8(tempD, tempC, tempB, tempA);     \
> -}
> -
> -SUBU_QB(subu, sub_u8);
> -SUBU_QB(subu_s, satu8_sub);
> -
> -#undef SUBU_QB
> -
>  #define SUBUH_QB(name, var) \
>  target_ulong helper_##name##_qb(target_ulong rs, target_ulong rt) \
>  {                                                                 \
> @@ -4027,7 +3876,6 @@ target_ulong helper_rddsp(target_ulong masknum, CPUMIPSState *env)
>  #undef MIPSDSP_SPLIT32_8
>  #undef MIPSDSP_SPLIT32_16
>
> -#undef MIPSDSP_RETURN32
>  #undef MIPSDSP_RETURN32_8
>  #undef MIPSDSP_RETURN32_16
>
> --
> 1.7.10.4
>
>
Aurelien Jarno Jan. 10, 2013, 7:08 a.m. UTC | #2
On Wed, Jan 09, 2013 at 09:16:29PM +0000, Blue Swirl wrote:
> On Wed, Jan 9, 2013 at 3:27 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > This allow to reduce the number of macros.
> >
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  target-mips/dsp_helper.c |  384 ++++++++++++++--------------------------------
> >  1 file changed, 116 insertions(+), 268 deletions(-)
> >
> > diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> > index aed4c63..e01c8a9 100644
> > --- a/target-mips/dsp_helper.c
> > +++ b/target-mips/dsp_helper.c
> > @@ -1078,7 +1078,6 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
> >          b = num & MIPSDSP_LO;               \
> >      } while (0)
> >
> > -#define MIPSDSP_RETURN32(a)             ((target_long)(int32_t)a)
> >  #define MIPSDSP_RETURN32_8(a, b, c, d)  ((target_long)(int32_t) \
> >                                           (((uint32_t)a << 24) | \
> >                                           (((uint32_t)b << 16) | \
> > @@ -1111,119 +1110,127 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
> >  #endif
> >
> >  /** DSP Arithmetic Sub-class insns **/
> > -#define ARITH_PH(name, func)                                      \
> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt) \
> > -{                                                                 \
> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
> > -                                                                  \
> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
> > -                                                                  \
> > -    temph = mipsdsp_##func(rsh, rth);                             \
> > -    templ = mipsdsp_##func(rsl, rtl);                             \
> > -                                                                  \
> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
> > -}
> > -
> > -#define ARITH_PH_ENV(name, func)                                  \
> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt, \
> > -                                CPUMIPSState *env)                \
> > -{                                                                 \
> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
> > -                                                                  \
> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
> > -                                                                  \
> > -    temph = mipsdsp_##func(rsh, rth, env);                        \
> > -    templ = mipsdsp_##func(rsl, rtl, env);                        \
> > -                                                                  \
> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
> > -}
> > -
> > -
> > -ARITH_PH_ENV(addq, add_i16);
> > -ARITH_PH_ENV(addq_s, sat_add_i16);
> > -ARITH_PH_ENV(addu, add_u16);
> > -ARITH_PH_ENV(addu_s, sat_add_u16);
> > -
> > -ARITH_PH(addqh, rshift1_add_q16);
> > -ARITH_PH(addqh_r, rrshift1_add_q16);
> > -
> > -ARITH_PH_ENV(subq, sub_i16);
> > -ARITH_PH_ENV(subq_s, sat16_sub);
> > -ARITH_PH_ENV(subu, sub_u16_u16);
> > -ARITH_PH_ENV(subu_s, satu16_sub_u16_u16);
> > -
> > -ARITH_PH(subqh, rshift1_sub_q16);
> > -ARITH_PH(subqh_r, rrshift1_sub_q16);
> > -
> > -#undef ARITH_PH
> > -#undef ARITH_PH_ENV
> > +#define MIPSDSP32_BINOP(name, func, element)                               \
> > +target_ulong helper_##name(target_ulong rs, target_ulong rt)               \
> > +{                                                                          \
> > +    DSP32Value ds, dt;                                                     \
> > +    unsigned int i, n;                                                     \
> > +                                                                           \
> > +    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                        \
> > +    ds.sw[0] = rs;                                                         \
> > +    dt.sw[0] = rt;                                                         \
> > +                                                                           \
> > +    for (i = 0 ; i < n ; i++) {                                            \
> 
> There's an extra space before ';', please remove. Also in the other
> for loops below.

It is not something I can find in CODING_STYLE, and it is also not
caught by checkpatch.pl.
Blue Swirl Jan. 12, 2013, 10:39 a.m. UTC | #3
On Thu, Jan 10, 2013 at 7:08 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Wed, Jan 09, 2013 at 09:16:29PM +0000, Blue Swirl wrote:
>> On Wed, Jan 9, 2013 at 3:27 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> > This allow to reduce the number of macros.
>> >
>> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> > ---
>> >  target-mips/dsp_helper.c |  384 ++++++++++++++--------------------------------
>> >  1 file changed, 116 insertions(+), 268 deletions(-)
>> >
>> > diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
>> > index aed4c63..e01c8a9 100644
>> > --- a/target-mips/dsp_helper.c
>> > +++ b/target-mips/dsp_helper.c
>> > @@ -1078,7 +1078,6 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
>> >          b = num & MIPSDSP_LO;               \
>> >      } while (0)
>> >
>> > -#define MIPSDSP_RETURN32(a)             ((target_long)(int32_t)a)
>> >  #define MIPSDSP_RETURN32_8(a, b, c, d)  ((target_long)(int32_t) \
>> >                                           (((uint32_t)a << 24) | \
>> >                                           (((uint32_t)b << 16) | \
>> > @@ -1111,119 +1110,127 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
>> >  #endif
>> >
>> >  /** DSP Arithmetic Sub-class insns **/
>> > -#define ARITH_PH(name, func)                                      \
>> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt) \
>> > -{                                                                 \
>> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
>> > -                                                                  \
>> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
>> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
>> > -                                                                  \
>> > -    temph = mipsdsp_##func(rsh, rth);                             \
>> > -    templ = mipsdsp_##func(rsl, rtl);                             \
>> > -                                                                  \
>> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
>> > -}
>> > -
>> > -#define ARITH_PH_ENV(name, func)                                  \
>> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt, \
>> > -                                CPUMIPSState *env)                \
>> > -{                                                                 \
>> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
>> > -                                                                  \
>> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
>> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
>> > -                                                                  \
>> > -    temph = mipsdsp_##func(rsh, rth, env);                        \
>> > -    templ = mipsdsp_##func(rsl, rtl, env);                        \
>> > -                                                                  \
>> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
>> > -}
>> > -
>> > -
>> > -ARITH_PH_ENV(addq, add_i16);
>> > -ARITH_PH_ENV(addq_s, sat_add_i16);
>> > -ARITH_PH_ENV(addu, add_u16);
>> > -ARITH_PH_ENV(addu_s, sat_add_u16);
>> > -
>> > -ARITH_PH(addqh, rshift1_add_q16);
>> > -ARITH_PH(addqh_r, rrshift1_add_q16);
>> > -
>> > -ARITH_PH_ENV(subq, sub_i16);
>> > -ARITH_PH_ENV(subq_s, sat16_sub);
>> > -ARITH_PH_ENV(subu, sub_u16_u16);
>> > -ARITH_PH_ENV(subu_s, satu16_sub_u16_u16);
>> > -
>> > -ARITH_PH(subqh, rshift1_sub_q16);
>> > -ARITH_PH(subqh_r, rrshift1_sub_q16);
>> > -
>> > -#undef ARITH_PH
>> > -#undef ARITH_PH_ENV
>> > +#define MIPSDSP32_BINOP(name, func, element)                               \
>> > +target_ulong helper_##name(target_ulong rs, target_ulong rt)               \
>> > +{                                                                          \
>> > +    DSP32Value ds, dt;                                                     \
>> > +    unsigned int i, n;                                                     \
>> > +                                                                           \
>> > +    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                        \
>> > +    ds.sw[0] = rs;                                                         \
>> > +    dt.sw[0] = rt;                                                         \
>> > +                                                                           \
>> > +    for (i = 0 ; i < n ; i++) {                                            \
>>
>> There's an extra space before ';', please remove. Also in the other
>> for loops below.
>
> It is not something I can find in CODING_STYLE, and it is also not
> caught by checkpatch.pl.

No, but it's not common style by far:
egrep -r '--exclude-dir=obj-*' '--exclude-dir=.git*'
'--exclude-dir=roms' '--exclude-dir=pc-bios' '--exclude-dir=pixman'
'--include=*.c' 'for.* ;' .|wc -l
74
egrep -r '--exclude-dir=obj-*' '--exclude-dir=.git*'
'--exclude-dir=roms' '--exclude-dir=pc-bios' '--exclude-dir=pixman'
'--include=*.c' 'for.*;' .|wc -l
4585

Original K&R style, from which QEMU style derives, didn't have the
spaces either. Perhaps you are influenced by French punctuation rules?

>
> --
> Aurelien Jarno                          GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net
Aurelien Jarno Jan. 12, 2013, 2:34 p.m. UTC | #4
On Sat, Jan 12, 2013 at 10:39:47AM +0000, Blue Swirl wrote:
> On Thu, Jan 10, 2013 at 7:08 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Wed, Jan 09, 2013 at 09:16:29PM +0000, Blue Swirl wrote:
> >> On Wed, Jan 9, 2013 at 3:27 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> >> > This allow to reduce the number of macros.
> >> >
> >> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> >> > ---
> >> >  target-mips/dsp_helper.c |  384 ++++++++++++++--------------------------------
> >> >  1 file changed, 116 insertions(+), 268 deletions(-)
> >> >
> >> > diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> >> > index aed4c63..e01c8a9 100644
> >> > --- a/target-mips/dsp_helper.c
> >> > +++ b/target-mips/dsp_helper.c
> >> > @@ -1078,7 +1078,6 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
> >> >          b = num & MIPSDSP_LO;               \
> >> >      } while (0)
> >> >
> >> > -#define MIPSDSP_RETURN32(a)             ((target_long)(int32_t)a)
> >> >  #define MIPSDSP_RETURN32_8(a, b, c, d)  ((target_long)(int32_t) \
> >> >                                           (((uint32_t)a << 24) | \
> >> >                                           (((uint32_t)b << 16) | \
> >> > @@ -1111,119 +1110,127 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
> >> >  #endif
> >> >
> >> >  /** DSP Arithmetic Sub-class insns **/
> >> > -#define ARITH_PH(name, func)                                      \
> >> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt) \
> >> > -{                                                                 \
> >> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
> >> > -                                                                  \
> >> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
> >> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
> >> > -                                                                  \
> >> > -    temph = mipsdsp_##func(rsh, rth);                             \
> >> > -    templ = mipsdsp_##func(rsl, rtl);                             \
> >> > -                                                                  \
> >> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
> >> > -}
> >> > -
> >> > -#define ARITH_PH_ENV(name, func)                                  \
> >> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt, \
> >> > -                                CPUMIPSState *env)                \
> >> > -{                                                                 \
> >> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
> >> > -                                                                  \
> >> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
> >> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
> >> > -                                                                  \
> >> > -    temph = mipsdsp_##func(rsh, rth, env);                        \
> >> > -    templ = mipsdsp_##func(rsl, rtl, env);                        \
> >> > -                                                                  \
> >> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
> >> > -}
> >> > -
> >> > -
> >> > -ARITH_PH_ENV(addq, add_i16);
> >> > -ARITH_PH_ENV(addq_s, sat_add_i16);
> >> > -ARITH_PH_ENV(addu, add_u16);
> >> > -ARITH_PH_ENV(addu_s, sat_add_u16);
> >> > -
> >> > -ARITH_PH(addqh, rshift1_add_q16);
> >> > -ARITH_PH(addqh_r, rrshift1_add_q16);
> >> > -
> >> > -ARITH_PH_ENV(subq, sub_i16);
> >> > -ARITH_PH_ENV(subq_s, sat16_sub);
> >> > -ARITH_PH_ENV(subu, sub_u16_u16);
> >> > -ARITH_PH_ENV(subu_s, satu16_sub_u16_u16);
> >> > -
> >> > -ARITH_PH(subqh, rshift1_sub_q16);
> >> > -ARITH_PH(subqh_r, rrshift1_sub_q16);
> >> > -
> >> > -#undef ARITH_PH
> >> > -#undef ARITH_PH_ENV
> >> > +#define MIPSDSP32_BINOP(name, func, element)                               \
> >> > +target_ulong helper_##name(target_ulong rs, target_ulong rt)               \
> >> > +{                                                                          \
> >> > +    DSP32Value ds, dt;                                                     \
> >> > +    unsigned int i, n;                                                     \
> >> > +                                                                           \
> >> > +    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                        \
> >> > +    ds.sw[0] = rs;                                                         \
> >> > +    dt.sw[0] = rt;                                                         \
> >> > +                                                                           \
> >> > +    for (i = 0 ; i < n ; i++) {                                            \
> >>
> >> There's an extra space before ';', please remove. Also in the other
> >> for loops below.
> >
> > It is not something I can find in CODING_STYLE, and it is also not
> > caught by checkpatch.pl.
> 
> No, but it's not common style by far:
> egrep -r '--exclude-dir=obj-*' '--exclude-dir=.git*'
> '--exclude-dir=roms' '--exclude-dir=pc-bios' '--exclude-dir=pixman'
> '--include=*.c' 'for.* ;' .|wc -l
> 74
> egrep -r '--exclude-dir=obj-*' '--exclude-dir=.git*'
> '--exclude-dir=roms' '--exclude-dir=pc-bios' '--exclude-dir=pixman'
> '--include=*.c' 'for.*;' .|wc -l
> 4585
> 
> Original K&R style, from which QEMU style derives, didn't have the
> spaces either. Perhaps you are influenced by French punctuation rules?

I don't really care if it is common or not. What I am saying is that if
you want a rule to be enforced, it's better to at least have it written.
It's also a good idea to have it added to checkpatch.pl, otherwise the
benefit of this tool is greatly reduced.
Blue Swirl Jan. 12, 2013, 3:08 p.m. UTC | #5
On Sat, Jan 12, 2013 at 2:34 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Sat, Jan 12, 2013 at 10:39:47AM +0000, Blue Swirl wrote:
>> On Thu, Jan 10, 2013 at 7:08 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> > On Wed, Jan 09, 2013 at 09:16:29PM +0000, Blue Swirl wrote:
>> >> On Wed, Jan 9, 2013 at 3:27 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> >> > This allow to reduce the number of macros.
>> >> >
>> >> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> >> > ---
>> >> >  target-mips/dsp_helper.c |  384 ++++++++++++++--------------------------------
>> >> >  1 file changed, 116 insertions(+), 268 deletions(-)
>> >> >
>> >> > diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
>> >> > index aed4c63..e01c8a9 100644
>> >> > --- a/target-mips/dsp_helper.c
>> >> > +++ b/target-mips/dsp_helper.c
>> >> > @@ -1078,7 +1078,6 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
>> >> >          b = num & MIPSDSP_LO;               \
>> >> >      } while (0)
>> >> >
>> >> > -#define MIPSDSP_RETURN32(a)             ((target_long)(int32_t)a)
>> >> >  #define MIPSDSP_RETURN32_8(a, b, c, d)  ((target_long)(int32_t) \
>> >> >                                           (((uint32_t)a << 24) | \
>> >> >                                           (((uint32_t)b << 16) | \
>> >> > @@ -1111,119 +1110,127 @@ static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
>> >> >  #endif
>> >> >
>> >> >  /** DSP Arithmetic Sub-class insns **/
>> >> > -#define ARITH_PH(name, func)                                      \
>> >> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt) \
>> >> > -{                                                                 \
>> >> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
>> >> > -                                                                  \
>> >> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
>> >> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
>> >> > -                                                                  \
>> >> > -    temph = mipsdsp_##func(rsh, rth);                             \
>> >> > -    templ = mipsdsp_##func(rsl, rtl);                             \
>> >> > -                                                                  \
>> >> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
>> >> > -}
>> >> > -
>> >> > -#define ARITH_PH_ENV(name, func)                                  \
>> >> > -target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt, \
>> >> > -                                CPUMIPSState *env)                \
>> >> > -{                                                                 \
>> >> > -    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
>> >> > -                                                                  \
>> >> > -    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
>> >> > -    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
>> >> > -                                                                  \
>> >> > -    temph = mipsdsp_##func(rsh, rth, env);                        \
>> >> > -    templ = mipsdsp_##func(rsl, rtl, env);                        \
>> >> > -                                                                  \
>> >> > -    return MIPSDSP_RETURN32_16(temph, templ);                     \
>> >> > -}
>> >> > -
>> >> > -
>> >> > -ARITH_PH_ENV(addq, add_i16);
>> >> > -ARITH_PH_ENV(addq_s, sat_add_i16);
>> >> > -ARITH_PH_ENV(addu, add_u16);
>> >> > -ARITH_PH_ENV(addu_s, sat_add_u16);
>> >> > -
>> >> > -ARITH_PH(addqh, rshift1_add_q16);
>> >> > -ARITH_PH(addqh_r, rrshift1_add_q16);
>> >> > -
>> >> > -ARITH_PH_ENV(subq, sub_i16);
>> >> > -ARITH_PH_ENV(subq_s, sat16_sub);
>> >> > -ARITH_PH_ENV(subu, sub_u16_u16);
>> >> > -ARITH_PH_ENV(subu_s, satu16_sub_u16_u16);
>> >> > -
>> >> > -ARITH_PH(subqh, rshift1_sub_q16);
>> >> > -ARITH_PH(subqh_r, rrshift1_sub_q16);
>> >> > -
>> >> > -#undef ARITH_PH
>> >> > -#undef ARITH_PH_ENV
>> >> > +#define MIPSDSP32_BINOP(name, func, element)                               \
>> >> > +target_ulong helper_##name(target_ulong rs, target_ulong rt)               \
>> >> > +{                                                                          \
>> >> > +    DSP32Value ds, dt;                                                     \
>> >> > +    unsigned int i, n;                                                     \
>> >> > +                                                                           \
>> >> > +    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                        \
>> >> > +    ds.sw[0] = rs;                                                         \
>> >> > +    dt.sw[0] = rt;                                                         \
>> >> > +                                                                           \
>> >> > +    for (i = 0 ; i < n ; i++) {                                            \
>> >>
>> >> There's an extra space before ';', please remove. Also in the other
>> >> for loops below.
>> >
>> > It is not something I can find in CODING_STYLE, and it is also not
>> > caught by checkpatch.pl.
>>
>> No, but it's not common style by far:
>> egrep -r '--exclude-dir=obj-*' '--exclude-dir=.git*'
>> '--exclude-dir=roms' '--exclude-dir=pc-bios' '--exclude-dir=pixman'
>> '--include=*.c' 'for.* ;' .|wc -l
>> 74
>> egrep -r '--exclude-dir=obj-*' '--exclude-dir=.git*'
>> '--exclude-dir=roms' '--exclude-dir=pc-bios' '--exclude-dir=pixman'
>> '--include=*.c' 'for.*;' .|wc -l
>> 4585
>>
>> Original K&R style, from which QEMU style derives, didn't have the
>> spaces either. Perhaps you are influenced by French punctuation rules?
>
> I don't really care if it is common or not.

But commonality is the whole point of the CODING_STYLE and
checkpatch.pl, we want to drive the code base to be more uniform. If
having extra spaces was very common, like 25% or more of 'for'
statements, it would be clear that there is no general rule but in
this case it's clearly not.

> What I am saying is that if
> you want a rule to be enforced, it's better to at least have it written.
> It's also a good idea to have it added to checkpatch.pl, otherwise the
> benefit of this tool is greatly reduced.

I'm pretty sure there are many things that a simple Perl script which
only sees a few lines being patched can't ever catch and CODING_STYLE
or other documents can't also cover all aspects of how to make code.

In this case, a rule for avoiding space before a semicolon (probably
also colons too) could be possible. To avoid false positives, it
should be allowed where it's the only thing on a line. Also, cases
like this:
/src/qemu/hw/spapr_hcall.c:133:        for (i = 0; ; ++i) {
where the second statement in the for loop is not present, it makes
the text even clearer.

>
> --
> Aurelien Jarno                          GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net
diff mbox

Patch

diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
index aed4c63..e01c8a9 100644
--- a/target-mips/dsp_helper.c
+++ b/target-mips/dsp_helper.c
@@ -1078,7 +1078,6 @@  static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
         b = num & MIPSDSP_LO;               \
     } while (0)
 
-#define MIPSDSP_RETURN32(a)             ((target_long)(int32_t)a)
 #define MIPSDSP_RETURN32_8(a, b, c, d)  ((target_long)(int32_t) \
                                          (((uint32_t)a << 24) | \
                                          (((uint32_t)b << 16) | \
@@ -1111,119 +1110,127 @@  static inline int32_t mipsdsp_cmpu_lt(uint32_t a, uint32_t b)
 #endif
 
 /** DSP Arithmetic Sub-class insns **/
-#define ARITH_PH(name, func)                                      \
-target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt) \
-{                                                                 \
-    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
-                                                                  \
-    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
-    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
-                                                                  \
-    temph = mipsdsp_##func(rsh, rth);                             \
-    templ = mipsdsp_##func(rsl, rtl);                             \
-                                                                  \
-    return MIPSDSP_RETURN32_16(temph, templ);                     \
-}
-
-#define ARITH_PH_ENV(name, func)                                  \
-target_ulong helper_##name##_ph(target_ulong rs, target_ulong rt, \
-                                CPUMIPSState *env)                \
-{                                                                 \
-    uint16_t  rsh, rsl, rth, rtl, temph, templ;                   \
-                                                                  \
-    MIPSDSP_SPLIT32_16(rs, rsh, rsl);                             \
-    MIPSDSP_SPLIT32_16(rt, rth, rtl);                             \
-                                                                  \
-    temph = mipsdsp_##func(rsh, rth, env);                        \
-    templ = mipsdsp_##func(rsl, rtl, env);                        \
-                                                                  \
-    return MIPSDSP_RETURN32_16(temph, templ);                     \
-}
-
-
-ARITH_PH_ENV(addq, add_i16);
-ARITH_PH_ENV(addq_s, sat_add_i16);
-ARITH_PH_ENV(addu, add_u16);
-ARITH_PH_ENV(addu_s, sat_add_u16);
-
-ARITH_PH(addqh, rshift1_add_q16);
-ARITH_PH(addqh_r, rrshift1_add_q16);
-
-ARITH_PH_ENV(subq, sub_i16);
-ARITH_PH_ENV(subq_s, sat16_sub);
-ARITH_PH_ENV(subu, sub_u16_u16);
-ARITH_PH_ENV(subu_s, satu16_sub_u16_u16);
-
-ARITH_PH(subqh, rshift1_sub_q16);
-ARITH_PH(subqh_r, rrshift1_sub_q16);
-
-#undef ARITH_PH
-#undef ARITH_PH_ENV
+#define MIPSDSP32_BINOP(name, func, element)                               \
+target_ulong helper_##name(target_ulong rs, target_ulong rt)               \
+{                                                                          \
+    DSP32Value ds, dt;                                                     \
+    unsigned int i, n;                                                     \
+                                                                           \
+    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                        \
+    ds.sw[0] = rs;                                                         \
+    dt.sw[0] = rt;                                                         \
+                                                                           \
+    for (i = 0 ; i < n ; i++) {                                            \
+        ds.element[i] = mipsdsp_##func(ds.element[i], dt.element[i]);      \
+    }                                                                      \
+                                                                           \
+    return (target_long)ds.sw[0];                                          \
+}
+MIPSDSP32_BINOP(addqh_ph, rshift1_add_q16, sh);
+MIPSDSP32_BINOP(addqh_r_ph, rrshift1_add_q16, sh);
+MIPSDSP32_BINOP(addqh_r_w, rrshift1_add_q32, sw);
+MIPSDSP32_BINOP(addqh_w, rshift1_add_q32, sw);
+MIPSDSP32_BINOP(adduh_qb, rshift1_add_u8, ub);
+MIPSDSP32_BINOP(adduh_r_qb, rrshift1_add_u8, ub);
+MIPSDSP32_BINOP(subqh_ph, rshift1_sub_q16, sh);
+MIPSDSP32_BINOP(subqh_r_ph, rrshift1_sub_q16, sh);
+MIPSDSP32_BINOP(subqh_r_w, rrshift1_sub_q32, sw);
+MIPSDSP32_BINOP(subqh_w, rshift1_sub_q32, sw);
+#undef MIPSDSP32_BINOP
+
+#define MIPSDSP32_BINOP_ENV(name, func, element)                           \
+target_ulong helper_##name(target_ulong rs, target_ulong rt,               \
+                           CPUMIPSState *env)                              \
+{                                                                          \
+    DSP32Value ds, dt;                                                     \
+    unsigned int i, n;                                                     \
+                                                                           \
+    n = sizeof(DSP32Value) / sizeof(ds.element[0]);                        \
+    ds.sw[0] = rs;                                                         \
+    dt.sw[0] = rt;                                                         \
+                                                                           \
+    for (i = 0 ; i < n ; i++) {                                            \
+        ds.element[i] = mipsdsp_##func(ds.element[i], dt.element[i], env); \
+    }                                                                      \
+                                                                           \
+    return (target_long)ds.sw[0];                                          \
+}
+MIPSDSP32_BINOP_ENV(addq_ph, add_i16, sh)
+MIPSDSP32_BINOP_ENV(addq_s_ph, sat_add_i16, sh)
+MIPSDSP32_BINOP_ENV(addq_s_w, sat_add_i32, sw);
+MIPSDSP32_BINOP_ENV(addu_ph, add_u16, sh)
+MIPSDSP32_BINOP_ENV(addu_qb, add_u8, ub);
+MIPSDSP32_BINOP_ENV(addu_s_ph, sat_add_u16, sh)
+MIPSDSP32_BINOP_ENV(addu_s_qb, sat_add_u8, ub);
+MIPSDSP32_BINOP_ENV(subq_ph, sub_i16, sh);
+MIPSDSP32_BINOP_ENV(subq_s_ph, sat16_sub, sh);
+MIPSDSP32_BINOP_ENV(subq_s_w, sat32_sub, sw);
+MIPSDSP32_BINOP_ENV(subu_ph, sub_u16_u16, sh);
+MIPSDSP32_BINOP_ENV(subu_qb, sub_u8, ub);
+MIPSDSP32_BINOP_ENV(subu_s_ph, satu16_sub_u16_u16, sh);
+MIPSDSP32_BINOP_ENV(subu_s_qb, satu8_sub, ub);
+#undef MIPSDSP32_BINOP_ENV
 
 #ifdef TARGET_MIPS64
-#define ARITH_QH_ENV(name, func) \
-target_ulong helper_##name##_qh(target_ulong rs, target_ulong rt, \
-                                CPUMIPSState *env)           \
-{                                                            \
-    uint16_t rs3, rs2, rs1, rs0;                             \
-    uint16_t rt3, rt2, rt1, rt0;                             \
-    uint16_t tempD, tempC, tempB, tempA;                     \
-                                                             \
-    MIPSDSP_SPLIT64_16(rs, rs3, rs2, rs1, rs0);              \
-    MIPSDSP_SPLIT64_16(rt, rt3, rt2, rt1, rt0);              \
-                                                             \
-    tempD = mipsdsp_##func(rs3, rt3, env);                   \
-    tempC = mipsdsp_##func(rs2, rt2, env);                   \
-    tempB = mipsdsp_##func(rs1, rt1, env);                   \
-    tempA = mipsdsp_##func(rs0, rt0, env);                   \
-                                                             \
-    return MIPSDSP_RETURN64_16(tempD, tempC, tempB, tempA);  \
-}
-
-ARITH_QH_ENV(addq, add_i16);
-ARITH_QH_ENV(addq_s, sat_add_i16);
-ARITH_QH_ENV(addu, add_u16);
-ARITH_QH_ENV(addu_s, sat_add_u16);
-
-ARITH_QH_ENV(subq, sub_i16);
-ARITH_QH_ENV(subq_s, sat16_sub);
-ARITH_QH_ENV(subu, sub_u16_u16);
-ARITH_QH_ENV(subu_s, satu16_sub_u16_u16);
-
-#undef ARITH_QH_ENV
+#define MIPSDSP64_BINOP(name, func, element)                               \
+target_ulong helper_##name(target_ulong rs, target_ulong rt)               \
+{                                                                          \
+    DSP64Value ds, dt;                                                     \
+    unsigned int i, n;                                                     \
+                                                                           \
+    n = sizeof(DSP64Value) / sizeof(ds.element[0]);                        \
+    ds.sl[0] = rs;                                                         \
+    dt.sl[0] = rt;                                                         \
+                                                                           \
+    for (i = 0 ; i < n ; i++) {                                            \
+        ds.element[i] = mipsdsp_##func(ds.element[i], dt.element[i]);      \
+    }                                                                      \
+                                                                           \
+    return ds.sl[0];                                                       \
+}
+MIPSDSP64_BINOP(adduh_ob, rshift1_add_u8, ub);
+MIPSDSP64_BINOP(adduh_r_ob, rrshift1_add_u8, ub);
+MIPSDSP64_BINOP(subuh_ob, rshift1_sub_u8, ub);
+MIPSDSP64_BINOP(subuh_r_ob, rrshift1_sub_u8, ub);
+#undef MIPSDSP64_BINOP
+
+#define MIPSDSP64_BINOP_ENV(name, func, element)                           \
+target_ulong helper_##name(target_ulong rs, target_ulong rt,               \
+                           CPUMIPSState *env)                              \
+{                                                                          \
+    DSP64Value ds, dt;                                                     \
+    unsigned int i, n;                                                     \
+                                                                           \
+    n = sizeof(DSP64Value) / sizeof(ds.element[0]);                        \
+    ds.sl[0] = rs;                                                         \
+    dt.sl[0] = rt;                                                         \
+                                                                           \
+    for (i = 0 ; i < n ; i++) {                                            \
+        ds.element[i] = mipsdsp_##func(ds.element[i], dt.element[i], env); \
+    }                                                                      \
+                                                                           \
+    return ds.sl[0];                                                       \
+}
+MIPSDSP64_BINOP_ENV(addq_pw, add_i32, sw);
+MIPSDSP64_BINOP_ENV(addq_qh, add_i16, sh);
+MIPSDSP64_BINOP_ENV(addq_s_pw, sat_add_i32, sw);
+MIPSDSP64_BINOP_ENV(addq_s_qh, sat_add_i16, sh);
+MIPSDSP64_BINOP_ENV(addu_ob, add_u8, uh);
+MIPSDSP64_BINOP_ENV(addu_qh, add_u16, uh);
+MIPSDSP64_BINOP_ENV(addu_s_ob, sat_add_u8, uh);
+MIPSDSP64_BINOP_ENV(addu_s_qh, sat_add_u16, uh);
+MIPSDSP64_BINOP_ENV(subq_pw, sub32, sw);
+MIPSDSP64_BINOP_ENV(subq_qh, sub_i16, sh);
+MIPSDSP64_BINOP_ENV(subq_s_pw, sat32_sub, sw);
+MIPSDSP64_BINOP_ENV(subq_s_qh, sat16_sub, sh);
+MIPSDSP64_BINOP_ENV(subu_ob, sub_u8, uh);
+MIPSDSP64_BINOP_ENV(subu_qh, sub_u16_u16, uh);
+MIPSDSP64_BINOP_ENV(subu_s_ob, satu8_sub, uh);
+MIPSDSP64_BINOP_ENV(subu_s_qh, satu16_sub_u16_u16, uh);
+#undef MIPSDSP64_BINOP_ENV
 
 #endif
 
-#define ARITH_W(name, func) \
-target_ulong helper_##name##_w(target_ulong rs, target_ulong rt) \
-{                                                                \
-    uint32_t rd;                                                 \
-    rd = mipsdsp_##func(rs, rt);                                 \
-    return MIPSDSP_RETURN32(rd);                                 \
-}
-
-#define ARITH_W_ENV(name, func) \
-target_ulong helper_##name##_w(target_ulong rs, target_ulong rt, \
-                               CPUMIPSState *env)                \
-{                                                                \
-    uint32_t rd;                                                 \
-    rd = mipsdsp_##func(rs, rt, env);                            \
-    return MIPSDSP_RETURN32(rd);                                 \
-}
-
-ARITH_W_ENV(addq_s, sat_add_i32);
-
-ARITH_W(addqh, rshift1_add_q32);
-ARITH_W(addqh_r, rrshift1_add_q32);
-
-ARITH_W_ENV(subq_s, sat32_sub);
-
-ARITH_W(subqh, rshift1_sub_q32);
-ARITH_W(subqh_r, rrshift1_sub_q32);
-
-#undef ARITH_W
-#undef ARITH_W_ENV
-
 target_ulong helper_absq_s_w(target_ulong rt, CPUMIPSState *env)
 {
     uint32_t rd;
@@ -1234,164 +1241,6 @@  target_ulong helper_absq_s_w(target_ulong rt, CPUMIPSState *env)
 }
 
 
-#if defined(TARGET_MIPS64)
-
-#define ARITH_PW_ENV(name, func) \
-target_ulong helper_##name##_pw(target_ulong rs, target_ulong rt, \
-                                CPUMIPSState *env)                \
-{                                                                 \
-    uint32_t rs1, rs0;                                            \
-    uint32_t rt1, rt0;                                            \
-    uint32_t tempB, tempA;                                        \
-                                                                  \
-    MIPSDSP_SPLIT64_32(rs, rs1, rs0);                             \
-    MIPSDSP_SPLIT64_32(rt, rt1, rt0);                             \
-                                                                  \
-    tempB = mipsdsp_##func(rs1, rt1, env);                        \
-    tempA = mipsdsp_##func(rs0, rt0, env);                        \
-                                                                  \
-    return MIPSDSP_RETURN64_32(tempB, tempA);                     \
-}
-
-ARITH_PW_ENV(addq, add_i32);
-ARITH_PW_ENV(addq_s, sat_add_i32);
-ARITH_PW_ENV(subq, sub32);
-ARITH_PW_ENV(subq_s, sat32_sub);
-
-#undef ARITH_PW_ENV
-
-#endif
-
-#define ARITH_QB(name, func) \
-target_ulong helper_##name##_qb(target_ulong rs, target_ulong rt) \
-{                                                                 \
-    uint8_t  rs0, rs1, rs2, rs3;                                  \
-    uint8_t  rt0, rt1, rt2, rt3;                                  \
-    uint8_t  temp0, temp1, temp2, temp3;                          \
-                                                                  \
-    MIPSDSP_SPLIT32_8(rs, rs3, rs2, rs1, rs0);                    \
-    MIPSDSP_SPLIT32_8(rt, rt3, rt2, rt1, rt0);                    \
-                                                                  \
-    temp0 = mipsdsp_##func(rs0, rt0);                             \
-    temp1 = mipsdsp_##func(rs1, rt1);                             \
-    temp2 = mipsdsp_##func(rs2, rt2);                             \
-    temp3 = mipsdsp_##func(rs3, rt3);                             \
-                                                                  \
-    return MIPSDSP_RETURN32_8(temp3, temp2, temp1, temp0);        \
-}
-
-#define ARITH_QB_ENV(name, func) \
-target_ulong helper_##name##_qb(target_ulong rs, target_ulong rt, \
-                                CPUMIPSState *env)          \
-{                                                           \
-    uint8_t  rs0, rs1, rs2, rs3;                            \
-    uint8_t  rt0, rt1, rt2, rt3;                            \
-    uint8_t  temp0, temp1, temp2, temp3;                    \
-                                                            \
-    MIPSDSP_SPLIT32_8(rs, rs3, rs2, rs1, rs0);              \
-    MIPSDSP_SPLIT32_8(rt, rt3, rt2, rt1, rt0);              \
-                                                            \
-    temp0 = mipsdsp_##func(rs0, rt0, env);                  \
-    temp1 = mipsdsp_##func(rs1, rt1, env);                  \
-    temp2 = mipsdsp_##func(rs2, rt2, env);                  \
-    temp3 = mipsdsp_##func(rs3, rt3, env);                  \
-                                                            \
-    return MIPSDSP_RETURN32_8(temp3, temp2, temp1, temp0);  \
-}
-
-ARITH_QB(adduh, rshift1_add_u8);
-ARITH_QB(adduh_r, rrshift1_add_u8);
-
-ARITH_QB_ENV(addu, add_u8);
-ARITH_QB_ENV(addu_s, sat_add_u8);
-
-#undef ADDU_QB
-#undef ADDU_QB_ENV
-
-#if defined(TARGET_MIPS64)
-#define ARITH_OB(name, func) \
-target_ulong helper_##name##_ob(target_ulong rs, target_ulong rt) \
-{                                                                 \
-    int i;                                                        \
-    uint8_t rs_t[8], rt_t[8];                                     \
-    uint8_t temp[8];                                              \
-    uint64_t result;                                              \
-                                                                  \
-    result = 0;                                                   \
-                                                                  \
-    for (i = 0; i < 8; i++) {                                     \
-        rs_t[i] = (rs >> (8 * i)) & MIPSDSP_Q0;                   \
-        rt_t[i] = (rt >> (8 * i)) & MIPSDSP_Q0;                   \
-        temp[i] = mipsdsp_##func(rs_t[i], rt_t[i]);               \
-        result |= (uint64_t)temp[i] << (8 * i);                   \
-    }                                                             \
-                                                                  \
-    return result;                                                \
-}
-
-#define ARITH_OB_ENV(name, func) \
-target_ulong helper_##name##_ob(target_ulong rs, target_ulong rt, \
-                                CPUMIPSState *env)                \
-{                                                                 \
-    int i;                                                        \
-    uint8_t rs_t[8], rt_t[8];                                     \
-    uint8_t temp[8];                                              \
-    uint64_t result;                                              \
-                                                                  \
-    result = 0;                                                   \
-                                                                  \
-    for (i = 0; i < 8; i++) {                                     \
-        rs_t[i] = (rs >> (8 * i)) & MIPSDSP_Q0;                   \
-        rt_t[i] = (rt >> (8 * i)) & MIPSDSP_Q0;                   \
-        temp[i] = mipsdsp_##func(rs_t[i], rt_t[i], env);          \
-        result |= (uint64_t)temp[i] << (8 * i);                   \
-    }                                                             \
-                                                                  \
-    return result;                                                \
-}
-
-ARITH_OB_ENV(addu, add_u8);
-ARITH_OB_ENV(addu_s, sat_add_u8);
-
-ARITH_OB(adduh, rshift1_add_u8);
-ARITH_OB(adduh_r, rrshift1_add_u8);
-
-ARITH_OB_ENV(subu, sub_u8);
-ARITH_OB_ENV(subu_s, satu8_sub);
-
-ARITH_OB(subuh, rshift1_sub_u8);
-ARITH_OB(subuh_r, rrshift1_sub_u8);
-
-#undef ARITH_OB
-#undef ARITH_OB_ENV
-
-#endif
-
-#define SUBU_QB(name, func) \
-target_ulong helper_##name##_qb(target_ulong rs,               \
-                                target_ulong rt,               \
-                                CPUMIPSState *env)             \
-{                                                              \
-    uint8_t rs3, rs2, rs1, rs0;                                \
-    uint8_t rt3, rt2, rt1, rt0;                                \
-    uint8_t tempD, tempC, tempB, tempA;                        \
-                                                               \
-    MIPSDSP_SPLIT32_8(rs, rs3, rs2, rs1, rs0);                 \
-    MIPSDSP_SPLIT32_8(rt, rt3, rt2, rt1, rt0);                 \
-                                                               \
-    tempD = mipsdsp_##func(rs3, rt3, env);                     \
-    tempC = mipsdsp_##func(rs2, rt2, env);                     \
-    tempB = mipsdsp_##func(rs1, rt1, env);                     \
-    tempA = mipsdsp_##func(rs0, rt0, env);                     \
-                                                               \
-    return MIPSDSP_RETURN32_8(tempD, tempC, tempB, tempA);     \
-}
-
-SUBU_QB(subu, sub_u8);
-SUBU_QB(subu_s, satu8_sub);
-
-#undef SUBU_QB
-
 #define SUBUH_QB(name, var) \
 target_ulong helper_##name##_qb(target_ulong rs, target_ulong rt) \
 {                                                                 \
@@ -4027,7 +3876,6 @@  target_ulong helper_rddsp(target_ulong masknum, CPUMIPSState *env)
 #undef MIPSDSP_SPLIT32_8
 #undef MIPSDSP_SPLIT32_16
 
-#undef MIPSDSP_RETURN32
 #undef MIPSDSP_RETURN32_8
 #undef MIPSDSP_RETURN32_16