qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Jia Liu
Subject: Re: [Qemu-devel] [PATCH v11 01/14] target-mips: Add ASE DSP internal functions
Date: Thu, 18 Oct 2012 09:53:20 +0800

Hi Aurelien,

On Wed, Oct 17, 2012 at 11:15 PM, Aurelien Jarno <address@hidden> 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 <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];
> |

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.


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

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

Regards,
Jia.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]