Re: [Qemu-devel] [PATCH v11 01/14] target-mips: Add ASE DSP internal functions

[Aurelien Jarno <aurelien@aurel32.net>]

On Thu, Oct 18, 2012 at 08:05:59AM +0200, Aurelien Jarno wrote:
> 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 <aurelien@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 <aurelien@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);
> |     }
> | }
> 

Actually this is likely wrong, MIPSDSP_OVERFLOW works for the overflow
detection, not for the carry detection. So something like that should
work:

| 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 ((uint64_t)env->active_tc.LO[ac] > (uint64_t)ret[0]) {
|         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
aurelien@aurel32.net                 http://www.aurel32.net