Re: [Qemu-devel] [PATCH V3 08/12] Add helper functions for MIPS DSP Bit/Manipulation instructions

[Jia Liu]

On Wed, Mar 28, 2012 at 12:23 AM, Richard Henderson <address@hidden> wrote:
> On 03/27/12 02:24, Jia Liu wrote:
>> +uint32_t helper_bitrev(uint32_t rt)
>> +{
>> +    int32_t temp;
>> +    uint32_t rd;
>> +
>> +    temp = rt & MIPSDSP_LO;
>> +    rd = temp;
>> +
>> +    return rd;
>> +}
>
> Forgot to actually reverse the bits.
>
>> +uint32_t helper_repl_qb(uint32_t imm)
> ...
>> +uint32_t helper_repl_ph(uint32_t imm)
>
> Should not be helpers.  The full 32-bit immediate can be
> computed directly in the translator.
>
>> +void helper_insv(int reg_rt, uint32_t rs, uint32_t rt)
>> +{
>> +    uint32_t pos, size, msb, lsb, rs_f, rt_f;
>> +    uint32_t temp, temprs, temprt;
>> +    target_ulong dspc;
>> +
>> +    dspc = env->active_tc.DSPControl;
>> +    pos  = dspc & 0x1F;
>> +    size = (dspc >> 7) & 0x1F;
>> +    msb  = pos + size - 1;
>> +    lsb  = pos;
>> +
>> +    if (lsb > msb)
>> +        return;
>> +
>> +    rs_f = (((int32_t)0x01 << (msb - lsb + 1 + 1)) - 1) << lsb;
>> +    rt_f = rs_f ^ 0xFFFFFFFF;
>> +    temprs = rs & rs_f;
>> +    temprt = rt & rt_f;
>> +    temp = temprs | temprt;
>> +    env->active_tc.gpr[reg_rt] = temp;
>> +}
>
> Err... why does this modify the env directly, rather than
> return a value?  "return rt" would seem to be the correct
> way to leave the value of reg_rt unchanged...
>
>
> r~

Yes, thank you Richard.
I'll fix them all.

Regards,
Jia.