On Thu, Oct 18, 2012 at 09:53:20AM +0800, Jia Liu wrote:
> Hi Aurelien,
>
> On Wed, Oct 17, 2012 at 11:15 PM, Aurelien Jarno <aurel...@aurel32.net> wrote:
> > On Wed, Oct 17, 2012 at 11:39:00AM +0800, Jia Liu wrote:
> >> Hi Aurelien,
> >>
> >> On Wed, Oct 17, 2012 at 7:20 AM, Aurelien Jarno <aurel...@aurel32.net>
> >> wrote:
> >> > On Tue, Oct 16, 2012 at 12:39:05AM +0800, Jia Liu wrote:
> >> >> +/* a[0] is LO, a[1] is HI. */
> >> >> +static inline void mipsdsp_sat64_acc_sub_q63(int64_t *ret,
> >> >> + int32_t ac,
> >> >> + int64_t *a,
> >> >> + CPUMIPSState *env)
> >> >> +{
> >> >> + uint32_t temp64, temp63;
> >> >> + int64_t temp[2];
> >> >> + int64_t acc[2];
> >> >> + int64_t temp_sum;
> >> >> +
> >> >> + temp[0] = a[0];
> >> >> + temp[1] = a[1];
> >> >> +
> >> >> + acc[0] = env->active_tc.LO[ac];
> >> >> + acc[1] = env->active_tc.HI[ac];
> >> >> +
> >> >> + temp_sum = acc[0] - temp[0];
> >> >> + if (MIPSDSP_OVERFLOW(acc[0], -temp[0], temp_sum,
> >> >> 0x8000000000000000ull)) {
> >> >> + acc[1] -= 1;
> >> >> + }
> >> >> + acc[0] = temp_sum;
> >> >> +
> >> >> + temp_sum = acc[1] - temp[1];
> >> >> + acc[1] = temp_sum;
> >> >> +
> >> >> + temp64 = acc[1] & 0x01;
> >> >> + temp63 = (acc[0] >> 63) & 0x01;
> >> >> +
> >> >> + /* MIPSDSP_OVERFLOW only can check if a 64 bits sub is overflow,
> >> >> + * there are two 128 bits value subed then check the 63/64 bits
> >> >> are equal
> >> >> + * or not.*/
> >> >
> >> > If you disagree with what I say, you can send mail, there is no need to
> >> > put it as a comment.
> >> >
> >> > That said MIPSDSP_OVERFLOW doesn't work only on 64-bit values, it can
> >> > work other size, as it is done elsewhere in this patch. The only thing
> >> > it checked is the highest bit of the two arguments and the result.
> >> > Therefore if you pass the highest part of the values, it can work.
> >> >
> >>
> >> I did agree with you, just didn't totally get your point.
> >>
> >> MIPSDSP_OVERFLOW used to check overflow, but here, 128bit + 128bit,
> >> low 64bit overflow need to be checked, so, in fact, I'm not sure what
> >> should do. Is this code right?
> >>
> >> static inline void mipsdsp_sat64_acc_sub_q63(uint64_t *ret,
> >> int32_t ac,
> >> uint64_t *a,
> >> CPUMIPSState *env)
> >> {
> >> uint32_t temp64;
> >> uint64_t temp[2];
> >> uint64_t acc[2];
> >>
> >> temp[0] = a[0];
> >> temp[1] = a[1];
> >>
> >> acc[0] = env->active_tc.LO[ac];
> >> acc[1] = env->active_tc.HI[ac];
> >>
> >> temp[1] = acc[1] - temp[1];
> >> temp[0] = acc[0] - temp[0];
> >>
> >> temp64 = temp[1] & 0x01;
> >>
> >> if (temp64 ^ MIPSDSP_OVERFLOW(acc[0], temp[0], temp[0], (0x01ull <<
> >> 63))) {
> >> if (temp64 == 1) {
> >> ret[0] = (0x01ull << 63);
> >> ret[1] = ~0ull;
> >> } else {
> >> ret[0] = (0x01ull << 63) - 1;
> >> ret[1] = 0x00;
> >> }
> >> set_DSPControl_overflow_flag(1, 16 + ac, env);
> >> } else {
> >> ret[0] = temp[0];
> >> ret[1] = acc[0] > temp[0] ? temp[1] : temp[1] - 1;
> >> }
> >> }
> >>
> >
> > I don't think xoring temp64 with MIPSDSP_OVERFLOW is correct. What about
> > about something like that (untested):
> >
> > | static inline void mipsdsp_sat64_acc_sub_q63(uint64_t *ret,
> > | int32_t ac,
> > | uint64_t *a,
> > | CPUMIPSState *env)
> > | {
> > | ret[0] = env->active_tc.LO[ac] - a[0];
> > | ret[1] = env->active_tc.HI[ac] - a[1];
> > |
>
> In the MIPS-DSP manual, the function is
>
> function sat64AccumulateSubQ63( acc1..0, a127..0 )
> temp128..0 ← HI[acc]63 || HI[acc]63..0 || LO[acc]63..0
> temp128..0 ← temp - a127..0 if ( temp64 ≠ temp63 ) then
> if ( temp64 = 1 ) then
> temp127..0 ← 0xFFFFFFFFFFFFFFFF8000000000000000
> else
> temp127..0 ← 0x00000000000000007FFFFFFFFFFFFFFF
> endif
> DSPControlouflag:16+acc ← 1 endif
> return temp127..0
> endfunction sat64AccumulateSubQ63
>
> > | if (MIPSDSP_OVERFLOW(env->active_tc.LO[ac], -a[0], ret[0],
> > 0x8000000000000000ull)) {
>
> The bit 64 will influence the overflow of bits 0-63.
> That is, if bit 64 is 1, and bits 0-63 overflowed, it won't be caught.
> So, if we use this code, it won't be handled.
>
I think you are correct, we have to take into account the overflow part,
but also the original value of the temp64 bit.
> > | if ((ret[1] - 1) & 1) {
> > | ret[0] = (0x01ull << 63);
> > | ret[1] = ~0ull;
> > | } else {
> > | ret[0] = (0x01ull << 63) - 1;
> > | ret[1] = 0x00;
> > | }
> > | set_DSPControl_overflow_flag(1, 16 + ac, env);
> > | }
> > | }
> > |
> >
> > The same applies for the add function, but also to some other places in
> > the code.
> >
> > Also note that you might want to have two version of MIPSDSP_OVERFLOW(),
> > one for add (like the current one), and one for sub (without the ^ -1),
> > so that you don't have to pass the negative value of the second
> > argument.
> >
>
> I think it is not necessary to define a new macro very much, what do
> you think about this code? Just little changed.
>
> /* a[0] is LO, a[1] is HI. */
> static inline void mipsdsp_sat64_acc_add_q63(int64_t *ret,
> int32_t ac,
> int64_t *a,
> CPUMIPSState *env)
> {
> uint32_t temp64;
>
> ret[0] = env->active_tc.LO[ac] + a[0];
> ret[1] = env->active_tc.HI[ac] + a[1];
>
> temp64 = ret[1] & 0x01;
>
> if (temp64 ^ MIPSDSP_OVERFLOW(env->active_tc.LO[ac], a[0], ret[0],
> (0x01ull << 63))) {
> if (temp64 == 1) {
> ret[0] = (0x01ull << 63);
> ret[1] = ~0ull;
> } else {
> ret[0] = (0x01ull << 63) - 1;
> ret[1] = 0x00;
> }
> set_DSPControl_overflow_flag(1, 16 + ac, env);
> } else {
> ret[1] = (((uint64_t)env->active_tc.LO[ac] > (uint64_t)ret[0]) &&
> ((uint64_t)a[0] > (uint64_t)ret[0])) ? ret[1] : ret[1] + 1;
> }
> }
>
> static inline void mipsdsp_sat64_acc_sub_q63(int64_t *ret,
> int32_t ac,
> int64_t *a,
> CPUMIPSState *env)
> {
> uint32_t temp64;
>
> ret[0] = env->active_tc.LO[ac] - a[0];
> ret[1] = env->active_tc.HI[ac] - a[1];
>
> temp64 = ret[1] & 0x01;
>
> if (temp64 ^ MIPSDSP_OVERFLOW(env->active_tc.LO[ac], -a[0], ret[0],
> (0x01ull << 63))) {
> if (temp64 == 1) {
> ret[0] = (0x01ull << 63);
> ret[1] = ~0ull;
> } else {
> ret[0] = (0x01ull << 63) - 1;
> ret[1] = 0x00;
> }
> set_DSPControl_overflow_flag(1, 16 + ac, env);
> } else {
> ret[1] = (uint64_t)env->active_tc.LO[ac] > (uint64_t)ret[0] ?
> ret[1] : ret[1] - 1;
> }
> }
>
That looks fine to me. That said given it mean we have to propagate the
carry, you might want to avoid the else case:
| static inline void mipsdsp_sat64_acc_sub_q63(int64_t *ret,
| int32_t ac,
| int64_t *a,
| CPUMIPSState *env)
| {
| bool temp64;
|
| ret[0] = env->active_tc.LO[ac] - a[0];
| ret[1] = env->active_tc.HI[ac] - a[1];
|
| if (MIPSDSP_OVERFLOW(env->active_tc.LO[ac], -a[0], ret[0],
| (0x01ull << 63))) {
| ret[1] -= 1;
| }
| temp64 = ret[1] & 1;
| if (temp64 != (ret[0] >> 63)) {
| if (temp64) {
| ret[0] = (0x01ull << 63);
| ret[1] = ~0ull;
| } else {
| ret[0] = (0x01ull << 63) - 1;
| ret[1] = 0x00;
| }
| set_DSPControl_overflow_flag(1, 16 + ac, env);
| }
| }
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net
