Message ID | 56EA75BA695AE044ACFB41322F6D2BF4022320A1@BADAG02.ba.imgtec.org |
---|---|
State | New |
Headers | show |
On 06/17/2013 03:39 PM, Petar Jovanovic wrote: > - int64_t temp; > - > - temp = (int32_t)a + 0x00008000; > + uint16_t temp; > > - if (a > (int)0x7fff8000) { > - temp = 0x7FFFFFFF; > + if (a > 0x7FFF7FFF) { > + temp = 0x7FFF; > set_DSPControl_overflow_flag(1, 22, env); > + } else { > + temp = ((a + 0x8000) >> 16) & 0xFFFF; This doesn't look right either, as it doesn't properly check for overflow of negative values. I'd feel better if we implement this function exactly as documented, modulo actually using 64-bit arithmetic. How about int32_t temp; /* Shift right by one, to avoid needing 64-bit arithmetic. As this A is signed, this creates the copy of the sign bit as documented. */ a >>= 1; temp = a + 0x4000; /* Compare temp{31} with temp{30} by xoring into the sign bit. */ if ((temp ^ (temp << 1)) < 0) { set_DSPControl_overflow_flag(1, 22, env); return 0x7fff; } return temp >> 15; r~
On 06/27/2013 02:48 PM, Petar Jovanovic wrote: >> This doesn't look right either, as it doesn't properly check for overflow of >> negative values. > > What overflow of negative values? > Can you please list the values for which the result would not be correct? Hmm, I suppose since we're always rounding to +INF, we can't overflow in the negative direction. The patch could use some commentary along those lines... r~
diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c index 4116de9..306b332 100644 --- a/target-mips/dsp_helper.c +++ b/target-mips/dsp_helper.c @@ -648,16 +648,16 @@ static inline int32_t mipsdsp_sat16_mul_q15_q15(uint16_t a, uint16_t b, static inline uint16_t mipsdsp_trunc16_sat16_round(int32_t a, CPUMIPSState *env) { - int64_t temp; - - temp = (int32_t)a + 0x00008000; + uint16_t temp; - if (a > (int)0x7fff8000) { - temp = 0x7FFFFFFF; + if (a > 0x7FFF7FFF) { + temp = 0x7FFF; set_DSPControl_overflow_flag(1, 22, env); + } else { + temp = ((a + 0x8000) >> 16) & 0xFFFF; } - return (temp >> 16) & 0xFFFF; + return temp; } static inline uint8_t mipsdsp_sat8_reduce_precision(uint16_t a, diff --git a/tests/tcg/mips/mips32-dsp/precrq_rs_ph_w.c b/tests/tcg/mips/mips32-dsp/precrq_rs_ph_w.c index 3535b37..da6845b 100644 --- a/tests/tcg/mips/mips32-dsp/precrq_rs_ph_w.c +++ b/tests/tcg/mips/mips32-dsp/precrq_rs_ph_w.c @@ -12,18 +12,34 @@ int main() result = 0x12348765; __asm - ("precrq_rs.ph.w %0, %1, %2\n\t" + ("wrdsp $0\n\t" + "precrq_rs.ph.w %0, %1, %2\n\t" : "=r"(rd) : "r"(rs), "r"(rt) ); assert(result == rd); - rs = 0x7fffC678; + rs = 0x7FFFC678; rt = 0x865432A0; - result = 0x7fff8654; + result = 0x7FFF8654; __asm - ("precrq_rs.ph.w %0, %2, %3\n\t" + ("wrdsp $0\n\t" + "precrq_rs.ph.w %0, %2, %3\n\t" + "rddsp %1\n\t" + : "=r"(rd), "=r"(dsp) + : "r"(rs), "r"(rt) + ); + assert(((dsp >> 22) & 0x01) == 1); + assert(result == rd); + + rs = 0xBEEFFEED; + rt = 0x7FFF8000; + result = 0xBEF07FFF; + + __asm + ("wrdsp $0\n\t" + "precrq_rs.ph.w %0, %2, %3\n\t" "rddsp %1\n\t" : "=r"(rd), "=r"(dsp) : "r"(rs), "r"(rt)