Re: [Qemu-devel] [PATCH v12 07/14] target-mips: Add ASE DSP GPR-based shift instructions

[Jia Liu]

Hi Aurelien,

On Mon, Oct 29, 2012 at 9:54 PM, Aurelien Jarno <address@hidden> wrote:
> On Wed, Oct 24, 2012 at 10:17:07PM +0800, Jia Liu wrote:
>> +static void gen_mipsdsp_shift(DisasContext *ctx, uint32_t opc,
>> +                              int ret, int v1, int v2)
>> +{
>> +    uint32_t op2;
>> +    const char *opn = "mipsdsp shift";
>> +    TCGv t0;
>> +    TCGv v1_t;
>> +    TCGv v2_t;
>> +
>> +    if (ret == 0) {
>> +        /* Treat as NOP. */
>> +        MIPS_DEBUG("NOP");
>> +        return;
>> +    }
>> +
>> +    t0 = tcg_temp_new();
>> +    v1_t = tcg_temp_new();
>> +    v2_t = tcg_temp_new();
>> +
>> +    tcg_gen_movi_tl(t0, v1);
>> +    gen_load_gpr(v1_t, v1);
>> +    gen_load_gpr(v2_t, v2);
>
> As previously said, this won't work. When v1 is an immediate value,
> gen_load_gpr() might be called with a value <= 0 or > 31. gen_load_gpr()
> is defined by:
>
> | static inline void gen_load_gpr (TCGv t, int reg)
> | {
> |     if (reg == 0)
> |         tcg_gen_movi_tl(t, 0);
> |     else
> |         tcg_gen_mov_tl(t, cpu_gpr[reg]);
> | }
>
> In that case, it means the cpu_gpr array will be accessed out of bound.
> The best solution for that is to split gen_mipsdsp_shift in two
> functions, one for register operations, and one for immediate
> operations.
>

ret/v1/v2 are all reg number, so they won't out of the range 0-31.
I just use tcg_gen_movi_tl(t0, v1); to transfer reg number to
dsp_helper.c to do more compute.

Need I add a comment here, or change the variable name?

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

Regards,
Jia.