* [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV @ 2012-12-04 23:29 Petar Jovanovic 2012-12-05 15:36 ` Richard Henderson ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Petar Jovanovic @ 2012-12-04 23:29 UTC (permalink / raw) To: qemu-devel; +Cc: blauwirbel, petarj, rth7680, afaerber, aurelien 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-04 23:29 [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV Petar Jovanovic @ 2012-12-05 15:36 ` Richard Henderson 2012-12-05 15:49 ` Peter Maydell 2012-12-05 20:41 ` Johnson, Eric 2012-12-06 8:11 ` Aurelien Jarno 2 siblings, 1 reply; 12+ messages in thread From: Richard Henderson @ 2012-12-05 15:36 UTC (permalink / raw) To: Petar Jovanovic Cc: rth7680, qemu-devel, blauwirbel, petarj, afaerber, aurelien 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~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-05 15:36 ` Richard Henderson @ 2012-12-05 15:49 ` Peter Maydell 2012-12-05 15:51 ` Richard Henderson 0 siblings, 1 reply; 12+ messages in thread From: Peter Maydell @ 2012-12-05 15:49 UTC (permalink / raw) To: Richard Henderson Cc: rth7680, Petar Jovanovic, qemu-devel, blauwirbel, petarj, afaerber, aurelien 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-05 15:49 ` Peter Maydell @ 2012-12-05 15:51 ` Richard Henderson 2012-12-05 16:38 ` Peter Maydell 0 siblings, 1 reply; 12+ messages in thread From: Richard Henderson @ 2012-12-05 15:51 UTC (permalink / raw) To: Peter Maydell Cc: rth7680, Petar Jovanovic, qemu-devel, blauwirbel, petarj, afaerber, aurelien 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~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-05 15:51 ` Richard Henderson @ 2012-12-05 16:38 ` Peter Maydell 2012-12-05 23:16 ` Richard Henderson 2012-12-06 8:17 ` Aurelien Jarno 0 siblings, 2 replies; 12+ messages in thread From: Peter Maydell @ 2012-12-05 16:38 UTC (permalink / raw) To: Richard Henderson Cc: rth7680, Petar Jovanovic, qemu-devel, blauwirbel, petarj, afaerber, aurelien 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-05 16:38 ` Peter Maydell @ 2012-12-05 23:16 ` Richard Henderson 2012-12-06 8:17 ` Aurelien Jarno 1 sibling, 0 replies; 12+ messages in thread From: Richard Henderson @ 2012-12-05 23:16 UTC (permalink / raw) To: Peter Maydell Cc: rth7680, Petar Jovanovic, qemu-devel, blauwirbel, petarj, afaerber, aurelien 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~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-05 16:38 ` Peter Maydell 2012-12-05 23:16 ` Richard Henderson @ 2012-12-06 8:17 ` Aurelien Jarno 1 sibling, 0 replies; 12+ messages in thread From: Aurelien Jarno @ 2012-12-06 8:17 UTC (permalink / raw) To: Peter Maydell Cc: rth7680, qemu-devel, Petar Jovanovic, blauwirbel, petarj, afaerber, Richard Henderson 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-04 23:29 [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV Petar Jovanovic 2012-12-05 15:36 ` Richard Henderson @ 2012-12-05 20:41 ` Johnson, Eric 2012-12-05 21:36 ` Johnson, Eric 2012-12-06 8:11 ` Aurelien Jarno 2 siblings, 1 reply; 12+ messages in thread From: Johnson, Eric @ 2012-12-05 20:41 UTC (permalink / raw) To: Jovanovic, Petar, qemu-devel@nongnu.org Cc: blauwirbel@gmail.com, rth7680@gmail.com, afaerber@suse.de, aurelien@aurel32.net > -----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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-05 20:41 ` Johnson, Eric @ 2012-12-05 21:36 ` Johnson, Eric 2012-12-05 23:31 ` Jovanovic, Petar 0 siblings, 1 reply; 12+ messages in thread From: Johnson, Eric @ 2012-12-05 21:36 UTC (permalink / raw) To: Jovanovic, Petar, qemu-devel@nongnu.org Cc: blauwirbel@gmail.com, rth7680@gmail.com, afaerber@suse.de, aurelien@aurel32.net 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-05 21:36 ` Johnson, Eric @ 2012-12-05 23:31 ` Jovanovic, Petar 2012-12-06 0:13 ` Johnson, Eric 0 siblings, 1 reply; 12+ messages in thread From: Jovanovic, Petar @ 2012-12-05 23:31 UTC (permalink / raw) To: Johnson, Eric, qemu-devel@nongnu.org Cc: blauwirbel@gmail.com, rth7680@gmail.com, afaerber@suse.de, aurelien@aurel32.net 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-05 23:31 ` Jovanovic, Petar @ 2012-12-06 0:13 ` Johnson, Eric 0 siblings, 0 replies; 12+ messages in thread From: Johnson, Eric @ 2012-12-06 0:13 UTC (permalink / raw) To: Jovanovic, Petar, qemu-devel@nongnu.org Cc: blauwirbel@gmail.com, rth7680@gmail.com, afaerber@suse.de, aurelien@aurel32.net 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV 2012-12-04 23:29 [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV Petar Jovanovic 2012-12-05 15:36 ` Richard Henderson 2012-12-05 20:41 ` Johnson, Eric @ 2012-12-06 8:11 ` Aurelien Jarno 2 siblings, 0 replies; 12+ messages in thread From: Aurelien Jarno @ 2012-12-06 8:11 UTC (permalink / raw) To: Petar Jovanovic Cc: rth7680, qemu-stable, qemu-devel, blauwirbel, petarj, afaerber 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-12-06 8:17 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-04 23:29 [Qemu-devel] [PATCH v2] target-mips: Fix incorrect shift for SHILO and SHILOV Petar Jovanovic 2012-12-05 15:36 ` Richard Henderson 2012-12-05 15:49 ` Peter Maydell 2012-12-05 15:51 ` Richard Henderson 2012-12-05 16:38 ` Peter Maydell 2012-12-05 23:16 ` Richard Henderson 2012-12-06 8:17 ` Aurelien Jarno 2012-12-05 20:41 ` Johnson, Eric 2012-12-05 21:36 ` Johnson, Eric 2012-12-05 23:31 ` Jovanovic, Petar 2012-12-06 0:13 ` Johnson, Eric 2012-12-06 8:11 ` Aurelien Jarno
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).