Patchwork [v2] target-mips: fix mipsdsp_trunc16_sat16_round

login
register
mail settings
Submitter Petar Jovanovic
Date June 30, 2013, 11:54 p.m.
Message ID <1372636487-124064-1-git-send-email-petar.jovanovic@rt-rk.com>
Download mbox | patch
Permalink /patch/255967/
State New
Headers show

Comments

Petar Jovanovic - June 30, 2013, 11:54 p.m.
From: Petar Jovanovic <petar.jovanovic@imgtec.com>

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 <petar.jovanovic@imgtec.com>
---

 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(-)
Richard Henderson - July 1, 2013, 2:52 p.m.
On 06/30/2013 04:54 PM, Petar Jovanovic wrote:
> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
> 
> 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 <petar.jovanovic@imgtec.com>
> ---
> 
>  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(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~
Petar Jovanovic - July 8, 2013, 9:14 a.m.
ping
http://patchwork.ozlabs.org/patch/255967/
Petar Jovanovic - July 15, 2013, 11:46 a.m.
ping
Petar Jovanovic - July 22, 2013, 8:51 a.m.
ping
Petar Jovanovic - July 27, 2013, 11:33 p.m.
ping

Can somebody submit these for 1.6?
Thank you.

Regards,
Petar
Aurelien Jarno - July 28, 2013, 10:28 p.m.
On Mon, Jul 01, 2013 at 01:54:47AM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
> 
> 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 <petar.jovanovic@imgtec.com>
> ---
> 
>  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.

Patch

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)