Re: [Qemu-devel] [PATCH v2] target-mips: fix mipsdsp_trunc16_sat16_round

[Aurelien Jarno]

On Mon, Jul 01, 2013 at 01:54:47AM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <address@hidden>
> 
> This change corrects rounding and saturation of Q31 fractional value in
> mipsdsp_trunc16_sat16_round(). Overflow detection was incorrect for the
> corner case for PRECRQ_RS.PH, and this test case is also part of the change.
> 
> Signed-off-by: Petar Jovanovic <address@hidden>
> ---
> 
>  v2:
> 
>  - added comments to the code
> 
>  target-mips/dsp_helper.c                   |   16 +++++++++++-----
>  tests/tcg/mips/mips32-dsp/precrq_rs_ph_w.c |   24 ++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index 4116de9..85950b3 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -648,16 +648,22 @@ 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;
> +    uint16_t temp;
>  
> -    temp = (int32_t)a + 0x00008000;
>  
> -    if (a > (int)0x7fff8000) {
> -        temp = 0x7FFFFFFF;
> +    /*
> +     * The value 0x00008000 will be added to the input Q31 value, and the 
> code
> +     * needs to check if the addition causes an overflow. Since a positive 
> value
> +     * is added, overflow can happen in one direction only.
> +     */
> +    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)

Thanks, applied.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net