[Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Petar Jovanovic]

From: Petar Jovanovic <petarj@mips.com>

helper_shilo has not been shifting an accumulator value correctly for negative
values in 'shift' field. Minor optimization for shift=0 case.
This change also adds tests that will trigger issue and check for regressions.

Signed-off-by: Petar Jovanovic <petarj@mips.com>
---
 target-mips/dsp_helper.c           |   17 +++++++++--------
 tests/tcg/mips/mips32-dsp/shilo.c  |   18 ++++++++++++++++++
 tests/tcg/mips/mips32-dsp/shilov.c |   20 ++++++++++++++++++++
 3 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
index e7949c2..44f6dc7 100644
--- a/target-mips/dsp_helper.c
+++ b/target-mips/dsp_helper.c
@@ -3814,17 +3814,18 @@ void helper_shilo(target_ulong ac, target_ulong rs, CPUMIPSState *env)
 
     rs5_0 = rs & 0x3F;
     rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
-    rs5_0 = MIPSDSP_ABS(rs5_0);
+
+    if (unlikely(rs5_0 == 0)) {
+        return;
+    }
+
     acc   = (((uint64_t)env->active_tc.HI[ac] << 32) & MIPSDSP_LHI) |
             ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
-    if (rs5_0 == 0) {
-        temp = acc;
+
+    if (rs5_0 > 0) {
+        temp = acc >> rs5_0;
     } else {
-        if (rs5_0 > 0) {
-            temp = acc >> rs5_0;
-        } else {
-            temp = acc << rs5_0;
-        }
+        temp = acc << -rs5_0;
     }
 
     env->active_tc.HI[ac] = (target_ulong)(int32_t)((temp & MIPSDSP_LHI) >> 32);
diff --git a/tests/tcg/mips/mips32-dsp/shilo.c b/tests/tcg/mips/mips32-dsp/shilo.c
index b686616..ce8ebc6 100644
--- a/tests/tcg/mips/mips32-dsp/shilo.c
+++ b/tests/tcg/mips/mips32-dsp/shilo.c
@@ -23,5 +23,23 @@ int main()
     assert(ach == resulth);
     assert(acl == resultl);
 
+
+    ach = 0x1;
+    acl = 0x80000000;
+
+    resulth = 0x3;
+    resultl = 0x0;
+
+    __asm
+        ("mthi %0, $ac1\n\t"
+         "mtlo %1, $ac1\n\t"
+         "shilo $ac1, -1\n\t"
+         "mfhi %0, $ac1\n\t"
+         "mflo %1, $ac1\n\t"
+         : "+r"(ach), "+r"(acl)
+        );
+    assert(ach == resulth);
+    assert(acl == resultl);
+
     return 0;
 }
diff --git a/tests/tcg/mips/mips32-dsp/shilov.c b/tests/tcg/mips/mips32-dsp/shilov.c
index f186032..e1d6cea 100644
--- a/tests/tcg/mips/mips32-dsp/shilov.c
+++ b/tests/tcg/mips/mips32-dsp/shilov.c
@@ -25,5 +25,25 @@ int main()
     assert(ach == resulth);
     assert(acl == resultl);
 
+
+    rs  = 0xffffffff;
+    ach = 0x1;
+    acl = 0x80000000;
+
+    resulth = 0x3;
+    resultl = 0x0;
+
+    __asm
+        ("mthi %0, $ac1\n\t"
+         "mtlo %1, $ac1\n\t"
+         "shilov $ac1, %2\n\t"
+         "mfhi %0, $ac1\n\t"
+         "mflo %1, $ac1\n\t"
+         : "+r"(ach), "+r"(acl)
+         : "r"(rs)
+        );
+    assert(ach == resulth);
+    assert(acl == resultl);
+
     return 0;
 }
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Richard Henderson]

On 2012-12-04 17:29, Petar Jovanovic wrote:
>      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;

This is more portably written as

  rs5_0 = (rs5_0 ^ 0x20) - 0x20;


r~

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Peter Maydell]

On 5 December 2012 15:36, Richard Henderson <rth@twiddle.net> wrote:
> On 2012-12-04 17:29, Petar Jovanovic wrote:
>>      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
>
> This is more portably written as
>
>   rs5_0 = (rs5_0 ^ 0x20) - 0x20;

...but way more obscurely. If we want to play that
kind of game can we have a sign-extension function in
a header somewhere?

-- PMM

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Richard Henderson]

On 2012-12-05 09:49, Peter Maydell wrote:
> On 5 December 2012 15:36, Richard Henderson <rth@twiddle.net> wrote:
>> On 2012-12-04 17:29, Petar Jovanovic wrote:
>>>      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
>>
>> This is more portably written as
>>
>>   rs5_0 = (rs5_0 ^ 0x20) - 0x20;
> 
> ...but way more obscurely. If we want to play that
> kind of game can we have a sign-extension function in
> a header somewhere?

I dunno about more obscurely.  It took me a minute to figure out
what was wanted in the original.

As for a helper function... sure.


r~

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Peter Maydell]

On 5 December 2012 15:51, Richard Henderson <rth@twiddle.net> wrote:
> On 2012-12-05 09:49, Peter Maydell wrote:
>> On 5 December 2012 15:36, Richard Henderson <rth@twiddle.net> wrote:
>>> On 2012-12-04 17:29, Petar Jovanovic wrote:
>>>>      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
>>>
>>> This is more portably written as
>>>
>>>   rs5_0 = (rs5_0 ^ 0x20) - 0x20;
>>
>> ...but way more obscurely. If we want to play that
>> kind of game can we have a sign-extension function in
>> a header somewhere?
>
> I dunno about more obscurely.  It took me a minute to figure out
> what was wanted in the original.
>
> As for a helper function... sure.

I don't think we should block this patch on that general
cleanup, though. All the sign extensions in target-mips/translate.c
are done in the double-shift way, so this is consistent with
the existing code.

-- PMM

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Johnson, Eric]

> -----Original Message-----
> From: qemu-devel-bounces+ericj=mips.com@nongnu.org [mailto:qemu-devel-
> bounces+ericj=mips.com@nongnu.org] On Behalf Of Petar Jovanovic
> Sent: Tuesday, December 04, 2012 3:29 PM
> To: qemu-devel@nongnu.org
> Cc: blauwirbel@gmail.com; Jovanovic, Petar; rth7680@gmail.com;
> afaerber@suse.de; aurelien@aurel32.net
> Subject: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for
> SHILO and SHILOV
> 
> From: Petar Jovanovic <petarj@mips.com>
> 
> helper_shilo has not been shifting an accumulator value correctly for
> negative
> values in 'shift' field. Minor optimization for shift=0 case.
> This change also adds tests that will trigger issue and check for
> regressions.
> 
> Signed-off-by: Petar Jovanovic <petarj@mips.com>
> ---
>  target-mips/dsp_helper.c           |   17 +++++++++--------
>  tests/tcg/mips/mips32-dsp/shilo.c  |   18 ++++++++++++++++++
>  tests/tcg/mips/mips32-dsp/shilov.c |   20 ++++++++++++++++++++
>  3 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index e7949c2..44f6dc7 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -3814,17 +3814,18 @@ void helper_shilo(target_ulong ac, target_ulong
> rs, CPUMIPSState *env)
> 
>      rs5_0 = rs & 0x3F;
>      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
> -    rs5_0 = MIPSDSP_ABS(rs5_0);
> +
> +    if (unlikely(rs5_0 == 0)) {
> +        return;
> +    }
> +
>      acc   = (((uint64_t)env->active_tc.HI[ac] << 32) & MIPSDSP_LHI) |
>              ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
> -    if (rs5_0 == 0) {
> -        temp = acc;
> +
> +    if (rs5_0 > 0) {
> +        temp = acc >> rs5_0;
>      } else {
> -        if (rs5_0 > 0) {
> -            temp = acc >> rs5_0;
> -        } else {
> -            temp = acc << rs5_0;
> -        }
> +        temp = acc << -rs5_0;
>      }
> 
>      env->active_tc.HI[ac] = (target_ulong)(int32_t)((temp & MIPSDSP_LHI)
> >> 32);
> diff --git a/tests/tcg/mips/mips32-dsp/shilo.c b/tests/tcg/mips/mips32-
> dsp/shilo.c
> index b686616..ce8ebc6 100644
> --- a/tests/tcg/mips/mips32-dsp/shilo.c
> +++ b/tests/tcg/mips/mips32-dsp/shilo.c
> @@ -23,5 +23,23 @@ int main()
>      assert(ach == resulth);
>      assert(acl == resultl);
> 
> +
> +    ach = 0x1;
> +    acl = 0x80000000;
> +
> +    resulth = 0x3;
> +    resultl = 0x0;
> +
> +    __asm
> +        ("mthi %0, $ac1\n\t"
> +         "mtlo %1, $ac1\n\t"
> +         "shilo $ac1, -1\n\t"
> +         "mfhi %0, $ac1\n\t"
> +         "mflo %1, $ac1\n\t"
> +         : "+r"(ach), "+r"(acl)
> +        );
> +    assert(ach == resulth);
> +    assert(acl == resultl);
> +
>      return 0;
>  }
> diff --git a/tests/tcg/mips/mips32-dsp/shilov.c b/tests/tcg/mips/mips32-
> dsp/shilov.c
> index f186032..e1d6cea 100644
> --- a/tests/tcg/mips/mips32-dsp/shilov.c
> +++ b/tests/tcg/mips/mips32-dsp/shilov.c
> @@ -25,5 +25,25 @@ int main()
>      assert(ach == resulth);
>      assert(acl == resultl);
> 
> +
> +    rs  = 0xffffffff;
> +    ach = 0x1;
> +    acl = 0x80000000;
> +
> +    resulth = 0x3;
> +    resultl = 0x0;
> +
> +    __asm
> +        ("mthi %0, $ac1\n\t"
> +         "mtlo %1, $ac1\n\t"
> +         "shilov $ac1, %2\n\t"
> +         "mfhi %0, $ac1\n\t"
> +         "mflo %1, $ac1\n\t"
> +         : "+r"(ach), "+r"(acl)
> +         : "r"(rs)
> +        );
> +    assert(ach == resulth);
> +    assert(acl == resultl);
> +
>      return 0;
>  }
> --
> 1.7.5.4
> 

Reviewed-by: Eric Johnson <ericj@mips.com>

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Johnson, Eric]

Oops, I forgot.  The contents are OK but 'git am' didn't like the patch.  patch had to use fuzz.  This may need to be rebased to latest master.

-Eric

> -----Original Message-----
> From: qemu-devel-bounces+ericj=mips.com@nongnu.org [mailto:qemu-devel-
> bounces+ericj=mips.com@nongnu.org] On Behalf Of Johnson, Eric
> Sent: Wednesday, December 05, 2012 12:42 PM
> To: Jovanovic, Petar; qemu-devel@nongnu.org
> Cc: blauwirbel@gmail.com; rth7680@gmail.com; afaerber@suse.de;
> aurelien@aurel32.net
> Subject: Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for
> SHILO and SHILOV
> 
> > -----Original Message-----
> > From: qemu-devel-bounces+ericj=mips.com@nongnu.org [mailto:qemu-devel-
> > bounces+ericj=mips.com@nongnu.org] On Behalf Of Petar Jovanovic
> > Sent: Tuesday, December 04, 2012 3:29 PM
> > To: qemu-devel@nongnu.org
> > Cc: blauwirbel@gmail.com; Jovanovic, Petar; rth7680@gmail.com;
> > afaerber@suse.de; aurelien@aurel32.net
> > Subject: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for
> > SHILO and SHILOV
> >
> > From: Petar Jovanovic <petarj@mips.com>
> >
> > helper_shilo has not been shifting an accumulator value correctly for
> > negative
> > values in 'shift' field. Minor optimization for shift=0 case.
> > This change also adds tests that will trigger issue and check for
> > regressions.
> >
> > Signed-off-by: Petar Jovanovic <petarj@mips.com>
> > ---
> >  target-mips/dsp_helper.c           |   17 +++++++++--------
> >  tests/tcg/mips/mips32-dsp/shilo.c  |   18 ++++++++++++++++++
> >  tests/tcg/mips/mips32-dsp/shilov.c |   20 ++++++++++++++++++++
> >  3 files changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> > index e7949c2..44f6dc7 100644
> > --- a/target-mips/dsp_helper.c
> > +++ b/target-mips/dsp_helper.c
> > @@ -3814,17 +3814,18 @@ void helper_shilo(target_ulong ac, target_ulong
> > rs, CPUMIPSState *env)
> >
> >      rs5_0 = rs & 0x3F;
> >      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
> > -    rs5_0 = MIPSDSP_ABS(rs5_0);
> > +
> > +    if (unlikely(rs5_0 == 0)) {
> > +        return;
> > +    }
> > +
> >      acc   = (((uint64_t)env->active_tc.HI[ac] << 32) & MIPSDSP_LHI) |
> >              ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
> > -    if (rs5_0 == 0) {
> > -        temp = acc;
> > +
> > +    if (rs5_0 > 0) {
> > +        temp = acc >> rs5_0;
> >      } else {
> > -        if (rs5_0 > 0) {
> > -            temp = acc >> rs5_0;
> > -        } else {
> > -            temp = acc << rs5_0;
> > -        }
> > +        temp = acc << -rs5_0;
> >      }
> >
> >      env->active_tc.HI[ac] = (target_ulong)(int32_t)((temp &
> MIPSDSP_LHI)
> > >> 32);
> > diff --git a/tests/tcg/mips/mips32-dsp/shilo.c b/tests/tcg/mips/mips32-
> > dsp/shilo.c
> > index b686616..ce8ebc6 100644
> > --- a/tests/tcg/mips/mips32-dsp/shilo.c
> > +++ b/tests/tcg/mips/mips32-dsp/shilo.c
> > @@ -23,5 +23,23 @@ int main()
> >      assert(ach == resulth);
> >      assert(acl == resultl);
> >
> > +
> > +    ach = 0x1;
> > +    acl = 0x80000000;
> > +
> > +    resulth = 0x3;
> > +    resultl = 0x0;
> > +
> > +    __asm
> > +        ("mthi %0, $ac1\n\t"
> > +         "mtlo %1, $ac1\n\t"
> > +         "shilo $ac1, -1\n\t"
> > +         "mfhi %0, $ac1\n\t"
> > +         "mflo %1, $ac1\n\t"
> > +         : "+r"(ach), "+r"(acl)
> > +        );
> > +    assert(ach == resulth);
> > +    assert(acl == resultl);
> > +
> >      return 0;
> >  }
> > diff --git a/tests/tcg/mips/mips32-dsp/shilov.c b/tests/tcg/mips/mips32-
> > dsp/shilov.c
> > index f186032..e1d6cea 100644
> > --- a/tests/tcg/mips/mips32-dsp/shilov.c
> > +++ b/tests/tcg/mips/mips32-dsp/shilov.c
> > @@ -25,5 +25,25 @@ int main()
> >      assert(ach == resulth);
> >      assert(acl == resultl);
> >
> > +
> > +    rs  = 0xffffffff;
> > +    ach = 0x1;
> > +    acl = 0x80000000;
> > +
> > +    resulth = 0x3;
> > +    resultl = 0x0;
> > +
> > +    __asm
> > +        ("mthi %0, $ac1\n\t"
> > +         "mtlo %1, $ac1\n\t"
> > +         "shilov $ac1, %2\n\t"
> > +         "mfhi %0, $ac1\n\t"
> > +         "mflo %1, $ac1\n\t"
> > +         : "+r"(ach), "+r"(acl)
> > +         : "r"(rs)
> > +        );
> > +    assert(ach == resulth);
> > +    assert(acl == resultl);
> > +
> >      return 0;
> >  }
> > --
> > 1.7.5.4
> >
> 
> Reviewed-by: Eric Johnson <ericj@mips.com>

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Richard Henderson]

On 2012-12-05 10:38, Peter Maydell wrote:
> I don't think we should block this patch on that general
> cleanup, though. All the sign extensions in target-mips/translate.c
> are done in the double-shift way, so this is consistent with
> the existing code.

Fair enough.  The original can have my 

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


r~

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Jovanovic, Petar]

hey Eric,

the patch does not have to be rebased since no changes have been made to
these files since I created the patch.

You can try it on clean master with:

wget -O shilo.diff http://patchwork.ozlabs.org/patch/203744/mbox
git am ./shilo.diff

You probably have the previous (not yet committed) INSV patch on your branch
that creates an issue for you when you run 'git am'.

Petar
________________________________________
From: Johnson, Eric
Sent: Wednesday, December 05, 2012 10:36 PM
To: Jovanovic, Petar; qemu-devel@nongnu.org
Cc: blauwirbel@gmail.com; rth7680@gmail.com; afaerber@suse.de; aurelien@aurel32.net
Subject: RE: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

Oops, I forgot.  The contents are OK but 'git am' didn't like the patch.  patch had to use fuzz.  This may need to be rebased to latest master.

-Eric

> -----Original Message-----
> From: qemu-devel-bounces+ericj=mips.com@nongnu.org [mailto:qemu-devel-
> bounces+ericj=mips.com@nongnu.org] On Behalf Of Johnson, Eric
> Sent: Wednesday, December 05, 2012 12:42 PM
> To: Jovanovic, Petar; qemu-devel@nongnu.org
> Cc: blauwirbel@gmail.com; rth7680@gmail.com; afaerber@suse.de;
> aurelien@aurel32.net
> Subject: Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for
> SHILO and SHILOV
>
> > -----Original Message-----
> > From: qemu-devel-bounces+ericj=mips.com@nongnu.org [mailto:qemu-devel-
> > bounces+ericj=mips.com@nongnu.org] On Behalf Of Petar Jovanovic
> > Sent: Tuesday, December 04, 2012 3:29 PM
> > To: qemu-devel@nongnu.org
> > Cc: blauwirbel@gmail.com; Jovanovic, Petar; rth7680@gmail.com;
> > afaerber@suse.de; aurelien@aurel32.net
> > Subject: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for
> > SHILO and SHILOV
> >
> > From: Petar Jovanovic <petarj@mips.com>
> >
> > helper_shilo has not been shifting an accumulator value correctly for
> > negative
> > values in 'shift' field. Minor optimization for shift=0 case.
> > This change also adds tests that will trigger issue and check for
> > regressions.
> >
> > Signed-off-by: Petar Jovanovic <petarj@mips.com>
> > ---
> >  target-mips/dsp_helper.c           |   17 +++++++++--------
> >  tests/tcg/mips/mips32-dsp/shilo.c  |   18 ++++++++++++++++++
> >  tests/tcg/mips/mips32-dsp/shilov.c |   20 ++++++++++++++++++++
> >  3 files changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> > index e7949c2..44f6dc7 100644
> > --- a/target-mips/dsp_helper.c
> > +++ b/target-mips/dsp_helper.c
> > @@ -3814,17 +3814,18 @@ void helper_shilo(target_ulong ac, target_ulong
> > rs, CPUMIPSState *env)
> >
> >      rs5_0 = rs & 0x3F;
> >      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
> > -    rs5_0 = MIPSDSP_ABS(rs5_0);
> > +
> > +    if (unlikely(rs5_0 == 0)) {
> > +        return;
> > +    }
> > +
> >      acc   = (((uint64_t)env->active_tc.HI[ac] << 32) & MIPSDSP_LHI) |
> >              ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
> > -    if (rs5_0 == 0) {
> > -        temp = acc;
> > +
> > +    if (rs5_0 > 0) {
> > +        temp = acc >> rs5_0;
> >      } else {
> > -        if (rs5_0 > 0) {
> > -            temp = acc >> rs5_0;
> > -        } else {
> > -            temp = acc << rs5_0;
> > -        }
> > +        temp = acc << -rs5_0;
> >      }
> >
> >      env->active_tc.HI[ac] = (target_ulong)(int32_t)((temp &
> MIPSDSP_LHI)
> > >> 32);
> > diff --git a/tests/tcg/mips/mips32-dsp/shilo.c b/tests/tcg/mips/mips32-
> > dsp/shilo.c
> > index b686616..ce8ebc6 100644
> > --- a/tests/tcg/mips/mips32-dsp/shilo.c
> > +++ b/tests/tcg/mips/mips32-dsp/shilo.c
> > @@ -23,5 +23,23 @@ int main()
> >      assert(ach == resulth);
> >      assert(acl == resultl);
> >
> > +
> > +    ach = 0x1;
> > +    acl = 0x80000000;
> > +
> > +    resulth = 0x3;
> > +    resultl = 0x0;
> > +
> > +    __asm
> > +        ("mthi %0, $ac1\n\t"
> > +         "mtlo %1, $ac1\n\t"
> > +         "shilo $ac1, -1\n\t"
> > +         "mfhi %0, $ac1\n\t"
> > +         "mflo %1, $ac1\n\t"
> > +         : "+r"(ach), "+r"(acl)
> > +        );
> > +    assert(ach == resulth);
> > +    assert(acl == resultl);
> > +
> >      return 0;
> >  }
> > diff --git a/tests/tcg/mips/mips32-dsp/shilov.c b/tests/tcg/mips/mips32-
> > dsp/shilov.c
> > index f186032..e1d6cea 100644
> > --- a/tests/tcg/mips/mips32-dsp/shilov.c
> > +++ b/tests/tcg/mips/mips32-dsp/shilov.c
> > @@ -25,5 +25,25 @@ int main()
> >      assert(ach == resulth);
> >      assert(acl == resultl);
> >
> > +
> > +    rs  = 0xffffffff;
> > +    ach = 0x1;
> > +    acl = 0x80000000;
> > +
> > +    resulth = 0x3;
> > +    resultl = 0x0;
> > +
> > +    __asm
> > +        ("mthi %0, $ac1\n\t"
> > +         "mtlo %1, $ac1\n\t"
> > +         "shilov $ac1, %2\n\t"
> > +         "mfhi %0, $ac1\n\t"
> > +         "mflo %1, $ac1\n\t"
> > +         : "+r"(ach), "+r"(acl)
> > +         : "r"(rs)
> > +        );
> > +    assert(ach == resulth);
> > +    assert(acl == resultl);
> > +
> >      return 0;
> >  }
> > --
> > 1.7.5.4
> >
>
> Reviewed-by: Eric Johnson <ericj@mips.com>

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Johnson, Eric]

Sorry mail wrap issue.

> -----Original Message-----
> From: Jovanovic, Petar
> Sent: Wednesday, December 05, 2012 3:31 PM
> To: Johnson, Eric; qemu-devel@nongnu.org
> Cc: blauwirbel@gmail.com; rth7680@gmail.com; afaerber@suse.de;
> aurelien@aurel32.net
> Subject: RE: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for
> SHILO and SHILOV
> 
> hey Eric,
> 
> the patch does not have to be rebased since no changes have been made to
> these files since I created the patch.
> 
> You can try it on clean master with:
> 
> wget -O shilo.diff http://patchwork.ozlabs.org/patch/203744/mbox
> git am ./shilo.diff
> 
> You probably have the previous (not yet committed) INSV patch on your
> branch
> that creates an issue for you when you run 'git am'.
> 
> Petar
> ________________________________________
> From: Johnson, Eric
> Sent: Wednesday, December 05, 2012 10:36 PM
> To: Jovanovic, Petar; qemu-devel@nongnu.org
> Cc: blauwirbel@gmail.com; rth7680@gmail.com; afaerber@suse.de;
> aurelien@aurel32.net
> Subject: RE: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for
> SHILO and SHILOV
> 
> Oops, I forgot.  The contents are OK but 'git am' didn't like the patch.
> patch had to use fuzz.  This may need to be rebased to latest master.
> 
> -Eric
> 
> > -----Original Message-----
> > From: qemu-devel-bounces+ericj=mips.com@nongnu.org [mailto:qemu-devel-
> > bounces+ericj=mips.com@nongnu.org] On Behalf Of Johnson, Eric
> > Sent: Wednesday, December 05, 2012 12:42 PM
> > To: Jovanovic, Petar; qemu-devel@nongnu.org
> > Cc: blauwirbel@gmail.com; rth7680@gmail.com; afaerber@suse.de;
> > aurelien@aurel32.net
> > Subject: Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift
> for
> > SHILO and SHILOV
> >
> > > -----Original Message-----
> > > From: qemu-devel-bounces+ericj=mips.com@nongnu.org [mailto:qemu-devel-
> > > bounces+ericj=mips.com@nongnu.org] On Behalf Of Petar Jovanovic
> > > Sent: Tuesday, December 04, 2012 3:29 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: blauwirbel@gmail.com; Jovanovic, Petar; rth7680@gmail.com;
> > > afaerber@suse.de; aurelien@aurel32.net
> > > Subject: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for
> > > SHILO and SHILOV
> > >
> > > From: Petar Jovanovic <petarj@mips.com>
> > >
> > > helper_shilo has not been shifting an accumulator value correctly for
> > > negative
> > > values in 'shift' field. Minor optimization for shift=0 case.
> > > This change also adds tests that will trigger issue and check for
> > > regressions.
> > >
> > > Signed-off-by: Petar Jovanovic <petarj@mips.com>
> > > ---
> > >  target-mips/dsp_helper.c           |   17 +++++++++--------
> > >  tests/tcg/mips/mips32-dsp/shilo.c  |   18 ++++++++++++++++++
> > >  tests/tcg/mips/mips32-dsp/shilov.c |   20 ++++++++++++++++++++
> > >  3 files changed, 47 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> > > index e7949c2..44f6dc7 100644
> > > --- a/target-mips/dsp_helper.c
> > > +++ b/target-mips/dsp_helper.c
> > > @@ -3814,17 +3814,18 @@ void helper_shilo(target_ulong ac,
> target_ulong
> > > rs, CPUMIPSState *env)
> > >
> > >      rs5_0 = rs & 0x3F;
> > >      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
> > > -    rs5_0 = MIPSDSP_ABS(rs5_0);
> > > +
> > > +    if (unlikely(rs5_0 == 0)) {
> > > +        return;
> > > +    }
> > > +
> > >      acc   = (((uint64_t)env->active_tc.HI[ac] << 32) & MIPSDSP_LHI) |
> > >              ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
> > > -    if (rs5_0 == 0) {
> > > -        temp = acc;
> > > +
> > > +    if (rs5_0 > 0) {
> > > +        temp = acc >> rs5_0;
> > >      } else {
> > > -        if (rs5_0 > 0) {
> > > -            temp = acc >> rs5_0;
> > > -        } else {
> > > -            temp = acc << rs5_0;
> > > -        }
> > > +        temp = acc << -rs5_0;
> > >      }
> > >
> > >      env->active_tc.HI[ac] = (target_ulong)(int32_t)((temp &
> > MIPSDSP_LHI)
> > > >> 32);
> > > diff --git a/tests/tcg/mips/mips32-dsp/shilo.c
> b/tests/tcg/mips/mips32-
> > > dsp/shilo.c
> > > index b686616..ce8ebc6 100644
> > > --- a/tests/tcg/mips/mips32-dsp/shilo.c
> > > +++ b/tests/tcg/mips/mips32-dsp/shilo.c
> > > @@ -23,5 +23,23 @@ int main()
> > >      assert(ach == resulth);
> > >      assert(acl == resultl);
> > >
> > > +
> > > +    ach = 0x1;
> > > +    acl = 0x80000000;
> > > +
> > > +    resulth = 0x3;
> > > +    resultl = 0x0;
> > > +
> > > +    __asm
> > > +        ("mthi %0, $ac1\n\t"
> > > +         "mtlo %1, $ac1\n\t"
> > > +         "shilo $ac1, -1\n\t"
> > > +         "mfhi %0, $ac1\n\t"
> > > +         "mflo %1, $ac1\n\t"
> > > +         : "+r"(ach), "+r"(acl)
> > > +        );
> > > +    assert(ach == resulth);
> > > +    assert(acl == resultl);
> > > +
> > >      return 0;
> > >  }
> > > diff --git a/tests/tcg/mips/mips32-dsp/shilov.c
> b/tests/tcg/mips/mips32-
> > > dsp/shilov.c
> > > index f186032..e1d6cea 100644
> > > --- a/tests/tcg/mips/mips32-dsp/shilov.c
> > > +++ b/tests/tcg/mips/mips32-dsp/shilov.c
> > > @@ -25,5 +25,25 @@ int main()
> > >      assert(ach == resulth);
> > >      assert(acl == resultl);
> > >
> > > +
> > > +    rs  = 0xffffffff;
> > > +    ach = 0x1;
> > > +    acl = 0x80000000;
> > > +
> > > +    resulth = 0x3;
> > > +    resultl = 0x0;
> > > +
> > > +    __asm
> > > +        ("mthi %0, $ac1\n\t"
> > > +         "mtlo %1, $ac1\n\t"
> > > +         "shilov $ac1, %2\n\t"
> > > +         "mfhi %0, $ac1\n\t"
> > > +         "mflo %1, $ac1\n\t"
> > > +         : "+r"(ach), "+r"(acl)
> > > +         : "r"(rs)
> > > +        );
> > > +    assert(ach == resulth);
> > > +    assert(acl == resultl);
> > > +
> > >      return 0;
> > >  }
> > > --
> > > 1.7.5.4
> > >
> >
> > Reviewed-by: Eric Johnson <ericj@mips.com>

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Aurelien Jarno]

On Wed, Dec 05, 2012 at 12:29:10AM +0100, Petar Jovanovic wrote:
> From: Petar Jovanovic <petarj@mips.com>
> 
> helper_shilo has not been shifting an accumulator value correctly for negative
> values in 'shift' field. Minor optimization for shift=0 case.
> This change also adds tests that will trigger issue and check for regressions.
> 
> Signed-off-by: Petar Jovanovic <petarj@mips.com>
> ---
>  target-mips/dsp_helper.c           |   17 +++++++++--------
>  tests/tcg/mips/mips32-dsp/shilo.c  |   18 ++++++++++++++++++
>  tests/tcg/mips/mips32-dsp/shilov.c |   20 ++++++++++++++++++++
>  3 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index e7949c2..44f6dc7 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -3814,17 +3814,18 @@ void helper_shilo(target_ulong ac, target_ulong rs, CPUMIPSState *env)
>  
>      rs5_0 = rs & 0x3F;
>      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
> -    rs5_0 = MIPSDSP_ABS(rs5_0);
> +
> +    if (unlikely(rs5_0 == 0)) {
> +        return;
> +    }
> +
>      acc   = (((uint64_t)env->active_tc.HI[ac] << 32) & MIPSDSP_LHI) |
>              ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
> -    if (rs5_0 == 0) {
> -        temp = acc;
> +
> +    if (rs5_0 > 0) {
> +        temp = acc >> rs5_0;
>      } else {
> -        if (rs5_0 > 0) {
> -            temp = acc >> rs5_0;
> -        } else {
> -            temp = acc << rs5_0;
> -        }
> +        temp = acc << -rs5_0;
>      }
>  
>      env->active_tc.HI[ac] = (target_ulong)(int32_t)((temp & MIPSDSP_LHI) >> 32);
> diff --git a/tests/tcg/mips/mips32-dsp/shilo.c b/tests/tcg/mips/mips32-dsp/shilo.c
> index b686616..ce8ebc6 100644
> --- a/tests/tcg/mips/mips32-dsp/shilo.c
> +++ b/tests/tcg/mips/mips32-dsp/shilo.c
> @@ -23,5 +23,23 @@ int main()
>      assert(ach == resulth);
>      assert(acl == resultl);
>  
> +
> +    ach = 0x1;
> +    acl = 0x80000000;
> +
> +    resulth = 0x3;
> +    resultl = 0x0;
> +
> +    __asm
> +        ("mthi %0, $ac1\n\t"
> +         "mtlo %1, $ac1\n\t"
> +         "shilo $ac1, -1\n\t"
> +         "mfhi %0, $ac1\n\t"
> +         "mflo %1, $ac1\n\t"
> +         : "+r"(ach), "+r"(acl)
> +        );
> +    assert(ach == resulth);
> +    assert(acl == resultl);
> +
>      return 0;
>  }
> diff --git a/tests/tcg/mips/mips32-dsp/shilov.c b/tests/tcg/mips/mips32-dsp/shilov.c
> index f186032..e1d6cea 100644
> --- a/tests/tcg/mips/mips32-dsp/shilov.c
> +++ b/tests/tcg/mips/mips32-dsp/shilov.c
> @@ -25,5 +25,25 @@ int main()
>      assert(ach == resulth);
>      assert(acl == resultl);
>  
> +
> +    rs  = 0xffffffff;
> +    ach = 0x1;
> +    acl = 0x80000000;
> +
> +    resulth = 0x3;
> +    resultl = 0x0;
> +
> +    __asm
> +        ("mthi %0, $ac1\n\t"
> +         "mtlo %1, $ac1\n\t"
> +         "shilov $ac1, %2\n\t"
> +         "mfhi %0, $ac1\n\t"
> +         "mflo %1, $ac1\n\t"
> +         : "+r"(ach), "+r"(acl)
> +         : "r"(rs)
> +        );
> +    assert(ach == resulth);
> +    assert(acl == resultl);
> +
>      return 0;
>  }

Thanks, applied. I added a CC: to qemu-stable@nongnu.org, as it is
definitely stable material.

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

Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV

[Aurelien Jarno]

On Wed, Dec 05, 2012 at 04:38:22PM +0000, Peter Maydell wrote:
> On 5 December 2012 15:51, Richard Henderson <rth@twiddle.net> wrote:
> > On 2012-12-05 09:49, Peter Maydell wrote:
> >> On 5 December 2012 15:36, Richard Henderson <rth@twiddle.net> wrote:
> >>> On 2012-12-04 17:29, Petar Jovanovic wrote:
> >>>>      rs5_0 = (int8_t)(rs5_0 << 2) >> 2;
> >>>
> >>> This is more portably written as
> >>>
> >>>   rs5_0 = (rs5_0 ^ 0x20) - 0x20;
> >>
> >> ...but way more obscurely. If we want to play that
> >> kind of game can we have a sign-extension function in
> >> a header somewhere?
> >
> > I dunno about more obscurely.  It took me a minute to figure out
> > what was wanted in the original.
> >
> > As for a helper function... sure.
> 
> I don't think we should block this patch on that general
> cleanup, though. All the sign extensions in target-mips/translate.c
> are done in the double-shift way, so this is consistent with
> the existing code.
> 

While it might be a good idea to make QEMU even more portable, it should
be noticed that currently QEMU assumes that a left shift followed by the
same arithmetic right shift. This is the case in at least the alpha,
arm, m68k, mips, ppc, s390 and unicore32 targets, as well as for
implementing sign extension and division in TCG.


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