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

[Aurelien Jarno]

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 <address@hidden> 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];
|
|     if (MIPSDSP_OVERFLOW(env->active_tc.LO[ac], -a[0], ret[0], 
0x8000000000000000ull)) {
|         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.

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