[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target-mips: fix DSP overflow macro and affecte
|
From: |
Aurelien Jarno |
|
Subject: |
Re: [Qemu-devel] [PATCH] target-mips: fix DSP overflow macro and affected routines |
|
Date: |
Mon, 4 Mar 2013 18:37:34 +0100 |
|
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Feb 25, 2013 at 04:45:40PM +0100, Petar Jovanovic wrote:
> From: Petar Jovanovic <address@hidden>
>
> The previous implementation incorrectly used same macro to detect overflow
> for addition and subtraction. This patch makes distinction between these
> two, and creates separate macros. The affected routines are changed
> accordingly.
I guess with the current system, the idea was to pass either a value of
the opposite for add/sub. But this was not done correctly.
> This change also includes additions to the existing tests for SUBQ_S_PH and
> SUBQ_S_W that would trigger the fixed issue, and it removes dead code from
> the test file. The last test case in subq_s_w.c is a bug found/reported/
> isolated by Klaus Peichl from Dolby.
>
> Signed-off-by: Petar Jovanovic <address@hidden>
> ---
> target-mips/dsp_helper.c | 90
> ++++++++++++++++++---------------
> tests/tcg/mips/mips32-dsp/subq_s_ph.c | 22 +++++++-
> tests/tcg/mips/mips32-dsp/subq_s_w.c | 36 +++++++++----
> 3 files changed, 94 insertions(+), 54 deletions(-)
Thanks for the patch, applied.
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index 841f47b..ffa9396 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -44,7 +44,8 @@ typedef union {
>
> /*** MIPS DSP internal functions begin ***/
> #define MIPSDSP_ABS(x) (((x) >= 0) ? x : -x)
> -#define MIPSDSP_OVERFLOW(a, b, c, d) (!(!((a ^ b ^ -1) & (a ^ c) & d)))
> +#define MIPSDSP_OVERFLOW_ADD(a, b, c, d) (~(a ^ b) & (a ^ c) & d)
> +#define MIPSDSP_OVERFLOW_SUB(a, b, c, d) ((a ^ b) & (a ^ c) & d)
>
> static inline void set_DSPControl_overflow_flag(uint32_t flag, int position,
> CPUMIPSState *env)
> @@ -142,7 +143,7 @@ static inline int16_t mipsdsp_add_i16(int16_t a, int16_t
> b, CPUMIPSState *env)
>
> tempI = a + b;
>
> - if (MIPSDSP_OVERFLOW(a, b, tempI, 0x8000)) {
> + if (MIPSDSP_OVERFLOW_ADD(a, b, tempI, 0x8000)) {
> set_DSPControl_overflow_flag(1, 20, env);
> }
>
> @@ -156,7 +157,7 @@ static inline int16_t mipsdsp_sat_add_i16(int16_t a,
> int16_t b,
>
> tempS = a + b;
>
> - if (MIPSDSP_OVERFLOW(a, b, tempS, 0x8000)) {
> + if (MIPSDSP_OVERFLOW_ADD(a, b, tempS, 0x8000)) {
> if (a > 0) {
> tempS = 0x7FFF;
> } else {
> @@ -175,7 +176,7 @@ static inline int32_t mipsdsp_sat_add_i32(int32_t a,
> int32_t b,
>
> tempI = a + b;
>
> - if (MIPSDSP_OVERFLOW(a, b, tempI, 0x80000000)) {
> + if (MIPSDSP_OVERFLOW_ADD(a, b, tempI, 0x80000000)) {
> if (a > 0) {
> tempI = 0x7FFFFFFF;
> } else {
> @@ -858,7 +859,7 @@ static inline uint16_t mipsdsp_sub_i16(int16_t a, int16_t
> b, CPUMIPSState *env)
> int16_t temp;
>
> temp = a - b;
> - if (MIPSDSP_OVERFLOW(a, -b, temp, 0x8000)) {
> + if (MIPSDSP_OVERFLOW_SUB(a, b, temp, 0x8000)) {
> set_DSPControl_overflow_flag(1, 20, env);
> }
>
> @@ -871,8 +872,8 @@ static inline uint16_t mipsdsp_sat16_sub(int16_t a,
> int16_t b,
> int16_t temp;
>
> temp = a - b;
> - if (MIPSDSP_OVERFLOW(a, -b, temp, 0x8000)) {
> - if (a > 0) {
> + if (MIPSDSP_OVERFLOW_SUB(a, b, temp, 0x8000)) {
> + if (a >= 0) {
> temp = 0x7FFF;
> } else {
> temp = 0x8000;
> @@ -889,8 +890,8 @@ static inline uint32_t mipsdsp_sat32_sub(int32_t a,
> int32_t b,
> int32_t temp;
>
> temp = a - b;
> - if (MIPSDSP_OVERFLOW(a, -b, temp, 0x80000000)) {
> - if (a > 0) {
> + if (MIPSDSP_OVERFLOW_SUB(a, b, temp, 0x80000000)) {
> + if (a >= 0) {
> temp = 0x7FFFFFFF;
> } else {
> temp = 0x80000000;
> @@ -1004,7 +1005,7 @@ static inline uint32_t mipsdsp_sub32(int32_t a, int32_t
> b, CPUMIPSState *env)
> int32_t temp;
>
> temp = a - b;
> - if (MIPSDSP_OVERFLOW(a, -b, temp, 0x80000000)) {
> + if (MIPSDSP_OVERFLOW_SUB(a, b, temp, 0x80000000)) {
> set_DSPControl_overflow_flag(1, 20, env);
> }
>
> @@ -1017,7 +1018,7 @@ static inline int32_t mipsdsp_add_i32(int32_t a,
> int32_t b, CPUMIPSState *env)
>
> temp = a + b;
>
> - if (MIPSDSP_OVERFLOW(a, b, temp, 0x80000000)) {
> + if (MIPSDSP_OVERFLOW_ADD(a, b, temp, 0x80000000)) {
> set_DSPControl_overflow_flag(1, 20, env);
> }
>
> @@ -2488,37 +2489,42 @@ DP_QH(dpsq_s_w_qh, 0, 1);
> #endif
>
> #define DP_L_W(name, is_add) \
> -void helper_##name(uint32_t ac, target_ulong rs, target_ulong rt, \
> - CPUMIPSState *env) \
> -{ \
> - int32_t temp63; \
> - int64_t dotp, acc; \
> - uint64_t temp; \
> - \
> - dotp = mipsdsp_mul_q31_q31(ac, rs, rt, env); \
> - acc = ((uint64_t)env->active_tc.HI[ac] << 32) | \
> - ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO); \
> - if (!is_add) { \
> - dotp = -dotp; \
> - } \
> - \
> - temp = acc + dotp; \
> - if (MIPSDSP_OVERFLOW((uint64_t)acc, (uint64_t)dotp, temp, \
> - (0x01ull << 63))) { \
> - temp63 = (temp >> 63) & 0x01; \
> - if (temp63 == 1) { \
> - temp = (0x01ull << 63) - 1; \
> - } else { \
> - temp = 0x01ull << 63; \
> - } \
> - \
> - set_DSPControl_overflow_flag(1, 16 + ac, env); \
> - } \
> - \
> - env->active_tc.HI[ac] = (target_long)(int32_t) \
> - ((temp & MIPSDSP_LHI) >> 32); \
> - env->active_tc.LO[ac] = (target_long)(int32_t) \
> - (temp & MIPSDSP_LLO); \
> +void helper_##name(uint32_t ac, target_ulong rs, target_ulong rt, \
> + CPUMIPSState *env) \
> +{ \
> + int32_t temp63; \
> + int64_t dotp, acc; \
> + uint64_t temp; \
> + bool overflow; \
> + \
> + dotp = mipsdsp_mul_q31_q31(ac, rs, rt, env); \
> + acc = ((uint64_t)env->active_tc.HI[ac] << 32) | \
> + ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO); \
> + if (is_add) { \
> + temp = acc + dotp; \
> + overflow = MIPSDSP_OVERFLOW_ADD((uint64_t)acc, (uint64_t)dotp, \
> + temp, (0x01ull << 63)); \
> + } else { \
> + temp = acc - dotp; \
> + overflow = MIPSDSP_OVERFLOW_SUB((uint64_t)acc, (uint64_t)dotp, \
> + temp, (0x01ull << 63)); \
> + } \
> + \
> + if (overflow) { \
> + temp63 = (temp >> 63) & 0x01; \
> + if (temp63 == 1) { \
> + temp = (0x01ull << 63) - 1; \
> + } else { \
> + temp = 0x01ull << 63; \
> + } \
> + \
> + set_DSPControl_overflow_flag(1, 16 + ac, env); \
> + } \
> + \
> + env->active_tc.HI[ac] = (target_long)(int32_t) \
> + ((temp & MIPSDSP_LHI) >> 32); \
> + env->active_tc.LO[ac] = (target_long)(int32_t) \
> + (temp & MIPSDSP_LLO); \
> }
>
> DP_L_W(dpaq_sa_l_w, 1);
> diff --git a/tests/tcg/mips/mips32-dsp/subq_s_ph.c
> b/tests/tcg/mips/mips32-dsp/subq_s_ph.c
> index 8e36dad..64c89eb 100644
> --- a/tests/tcg/mips/mips32-dsp/subq_s_ph.c
> +++ b/tests/tcg/mips/mips32-dsp/subq_s_ph.c
> @@ -12,7 +12,8 @@ int main()
> resultdsp = 0x01;
>
> __asm
> - ("subq_s.ph %0, %2, %3\n\t"
> + ("wrdsp $0\n\t"
> + "subq_s.ph %0, %2, %3\n\t"
> "rddsp %1\n\t"
> : "=r"(rd), "=r"(dsp)
> : "r"(rs), "r"(rt)
> @@ -27,7 +28,24 @@ int main()
> resultdsp = 0x01;
>
> __asm
> - ("subq_s.ph %0, %2, %3\n\t"
> + ("wrdsp $0\n\t"
> + "subq_s.ph %0, %2, %3\n\t"
> + "rddsp %1\n\t"
> + : "=r"(rd), "=r"(dsp)
> + : "r"(rs), "r"(rt)
> + );
> + dsp = (dsp >> 20) & 0x01;
> + assert(dsp == resultdsp);
> + assert(rd == result);
> +
> + rs = 0x12340000;
> + rt = 0x87658000;
> + result = 0x7FFF7FFF;
> + resultdsp = 0x01;
> +
> + __asm
> + ("wrdsp $0\n\t"
> + "subq_s.ph %0, %2, %3\n\t"
> "rddsp %1\n\t"
> : "=r"(rd), "=r"(dsp)
> : "r"(rs), "r"(rt)
> diff --git a/tests/tcg/mips/mips32-dsp/subq_s_w.c
> b/tests/tcg/mips/mips32-dsp/subq_s_w.c
> index 09022e9..9d456a9 100644
> --- a/tests/tcg/mips/mips32-dsp/subq_s_w.c
> +++ b/tests/tcg/mips/mips32-dsp/subq_s_w.c
> @@ -12,7 +12,8 @@ int main()
> resultdsp = 0x01;
>
> __asm
> - ("subq_s.w %0, %2, %3\n\t"
> + ("wrdsp $0\n\t"
> + "subq_s.w %0, %2, %3\n\t"
> "rddsp %1\n\t"
> : "=r"(rd), "=r"(dsp)
> : "r"(rs), "r"(rt)
> @@ -24,10 +25,11 @@ int main()
> rs = 0x66666;
> rt = 0x55555;
> result = 0x11111;
> - resultdsp = 0x01;
> + resultdsp = 0x0;
>
> __asm
> - ("subq_s.w %0, %2, %3\n\t"
> + ("wrdsp $0\n\t"
> + "subq_s.w %0, %2, %3\n\t"
> "rddsp %1\n\t"
> : "=r"(rd), "=r"(dsp)
> : "r"(rs), "r"(rt)
> @@ -36,23 +38,37 @@ int main()
> assert(dsp == resultdsp);
> assert(rd == result);
>
> -
> -#if 0
> - rs = 0x35555555;
> - rt = 0xf5555555;
> - result = 0x80000000;
> + rs = 0x0;
> + rt = 0x80000000;
> + result = 0x7FFFFFFF;
> resultdsp = 0x01;
>
> __asm
> - ("subq_s.w %0, %2, %3\n\t"
> + ("wrdsp $0\n\t"
> + "subq_s.w %0, %2, %3\n\t"
> "rddsp %1\n\t"
> : "=r"(rd), "=r"(dsp)
> : "r"(rs), "r"(rt)
> );
> + dsp = (dsp >> 20) & 0x01;
> + assert(dsp == resultdsp);
> + assert(rd == result);
> +
> + rs = 0x80000000;
> + rt = 0x80000000;
> + result = 0;
> + resultdsp = 0x00;
>
> + __asm
> + ("wrdsp $0\n\t"
> + "subq_s.w %0, %2, %3\n\t"
> + "rddsp %1\n\t"
> + : "=r"(rd), "=r"(dsp)
> + : "r"(rs), "r"(rt)
> + );
> dsp = (dsp >> 20) & 0x01;
> assert(dsp == resultdsp);
> assert(rd == result);
> -#endif
> +
> return 0;
> }
> --
> 1.7.9.5
>
--
Aurelien Jarno GPG: 1024D/F1BCDB73
address@hidden http://www.aurel32.net
| [Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH] target-mips: fix DSP overflow macro and affected routines,
Aurelien Jarno <=