Patchwork [v2] target-mips: Fix incorrect shift for SHILO and SHILOV

login
register
mail settings
Submitter Petar Jovanovic
Date Dec. 4, 2012, 11:29 p.m.
Message ID <1354663750-2163-1-git-send-email-petar.jovanovic@rt-rk.com>
Download mbox | patch
Permalink /patch/203744/
State New
Headers show

Comments

Petar Jovanovic - Dec. 4, 2012, 11:29 p.m.
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(-)
Richard Henderson - Dec. 5, 2012, 3:36 p.m.
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~
Peter Maydell - Dec. 5, 2012, 3:49 p.m.
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
Richard Henderson - Dec. 5, 2012, 3:51 p.m.
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~
Peter Maydell - Dec. 5, 2012, 4:38 p.m.
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
Eric Johnson - Dec. 5, 2012, 8:41 p.m.
> -----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>
Eric Johnson - Dec. 5, 2012, 9:36 p.m.
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>
Richard Henderson - Dec. 5, 2012, 11:16 p.m.
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~
Jovanovic, Petar - Dec. 5, 2012, 11:31 p.m.
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
Eric Johnson - Dec. 6, 2012, 12:13 a.m.
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>
Aurelien Jarno - Dec. 6, 2012, 8:11 a.m.
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 - Dec. 6, 2012, 8:17 a.m.
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.

Patch

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;
 }