Message ID | 1361807140-60543-1-git-send-email-petar.jovanovic@rt-rk.com |
---|---|
State | New |
Headers | show |
On Mon, Feb 25, 2013 at 04:45:40PM +0100, Petar Jovanovic wrote: > From: Petar Jovanovic <petar.jovanovic@imgtec.com> > > 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 <petar.jovanovic@imgtec.com> > --- > 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 >
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; }