Re: [Qemu-devel] [PATCH v12 09/14] target-mips: Add ASE DSP bit/manipulation instructions

[Jia Liu]

Hi Richard Peter Jovanovic and Aurelien,

On Wed, Oct 31, 2012 at 1:26 PM, Richard Henderson <address@hidden> wrote:
> On 2012-10-31 01:44, Peter Maydell wrote:
>> On 30 October 2012 15:34, Jia Liu <address@hidden> wrote:
>>> On Mon, Oct 29, 2012 at 9:40 PM, Jovanovic, Petar <address@hidden> wrote:
>>>>> imm = (int16_t)(imm << 6) >> 6;
>>>>
>>>> result of a bitwise shift of a signed type and a negative vlaue is
>>>> implementation-defined, so you can not rely on that.
>>>>
>>>
>>> I think it will take a 10bits signed value sign extend into 16bits
>>> signed value, and I've tested it with negative values, it working
>>> well.
>>
>> You cannot rely on the behaviour of a specific compiler implementation
>> as evidence that a piece of code is correct. C has a standard which
>> defines what is and is not valid.
>
> Indeed.  The only portable way is
>
>   val = ((val & (sign | (sign - 1))) ^ sign) - sign
>
> with all unsigned types, and "sign" set to the sign bit.
>

Thank you very much for the code.

Is this time OK?

static void gen_mipsdsp_bitinsn(CPUMIPSState *env, DisasContext *ctx,
                                uint32_t op1, uint32_t op2,
                                int ret, int val)
{
    const char *opn = "mipsdsp Bit/ Manipulation";
    TCGv t0;
    TCGv val_t;

    if (ret == 0) {
        /* Treat as NOP. */
        MIPS_DEBUG("NOP");
        return;
    }

    t0 = tcg_temp_new();
    val_t = tcg_temp_new();
    gen_load_gpr(val_t, val);

#define SIGN_EXTEND10_16(val, sign) \
    val = (((val & (sign | (sign - 1))) ^ sign) - sign)

    switch (op1) {
    case OPC_ABSQ_S_PH_DSP:
        switch (op2) {
        case OPC_BITREV:
            check_dsp(ctx);
            gen_helper_bitrev(cpu_gpr[ret], val_t);
            break;
        case OPC_REPL_QB:
            check_dsp(ctx);
            {
                uint32_t imm;
                target_long result;

                imm = (ctx->opcode >> 16) & 0xFF;
                result = imm << 24 | imm << 16 | imm << 8  | imm;
                result = (int32_t)result;
                tcg_gen_movi_tl(cpu_gpr[ret], result);
            }
            break;
        case OPC_REPLV_QB:
            check_dsp(ctx);
            tcg_gen_ext8u_tl(cpu_gpr[ret], val_t);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 8);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 16);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            tcg_gen_ext32s_tl(cpu_gpr[ret], cpu_gpr[ret]);
            break;
        case OPC_REPL_PH:
            check_dsp(ctx);
            {
                uint16_t imm;
                imm = (ctx->opcode >> 16) & 0x03FF;
                SIGN_EXTEND10_16(imm, 0x200);
                tcg_gen_movi_tl(cpu_gpr[ret],
                                (target_long)(int32_t)
                                ((uint32_t)imm << 16 | imm));
            }
            break;
        case OPC_REPLV_PH:
            check_dsp(ctx);
            tcg_gen_ext16u_tl(cpu_gpr[ret], val_t);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 16);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            tcg_gen_ext32s_tl(cpu_gpr[ret], cpu_gpr[ret]);
            break;
        }
        break;
#ifdef TARGET_MIPS64
    case OPC_ABSQ_S_QH_DSP:
        switch (op2) {
        case OPC_REPL_OB:
            check_dsp(ctx);
            {
                target_ulong imm;

                imm = (ctx->opcode >> 16) & 0xFF;
                imm = (imm << 8) | imm;
                imm = (imm << 16) | imm;
                imm = (imm << 32) | imm;
                tcg_gen_movi_tl(cpu_gpr[ret], imm);
                break;
            }
        case OPC_REPL_PW:
            check_dsp(ctx);
            {
                target_long imm;

                imm = (ctx->opcode >> 16) & 0x03FF;
                SIGN_EXTEND10_16(imm, 0x200);
                tcg_gen_movi_tl(cpu_gpr[ret],
                                (imm << 32) | (imm & 0xFFFFFFFF));
                break;
            }
        case OPC_REPL_QH:
            check_dsp(ctx);
            {
                target_ulong imm;

                imm = (ctx->opcode >> 16) & 0x03FF;
                SIGN_EXTEND10_16(imm, 0x200);

                imm = imm & 0xFFFF;
                imm = (imm << 48) | (imm << 32) | (imm << 16) | imm;
                tcg_gen_movi_tl(cpu_gpr[ret], imm);
                break;
            }
        case OPC_REPLV_OB:
            check_dsp(ctx);
            tcg_gen_ext8u_tl(cpu_gpr[ret], val_t);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 8);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 16);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 32);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            break;
        case OPC_REPLV_PW:
            check_dsp(ctx);
            tcg_gen_ext32u_i64(cpu_gpr[ret], val_t);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 32);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            break;
        case OPC_REPLV_QH:
            check_dsp(ctx);
            tcg_gen_ext16u_tl(cpu_gpr[ret], val_t);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 16);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            tcg_gen_shli_tl(t0, cpu_gpr[ret], 32);
            tcg_gen_or_tl(cpu_gpr[ret], cpu_gpr[ret], t0);
            break;
        }
        break;
#endif
    }

#undef SIGN_EXTEND10_16
    tcg_temp_free(t0);
    tcg_temp_free(val_t);

    (void)opn; /* avoid a compiler warning */
    MIPS_DEBUG("%s", opn);
}


>>
>> Having said that, right shift of negative signed integers is one of
>> those bits of implementation defined behaviour which we allow ourselves
>> to rely on in QEMU because all the platforms we care about behave
>> that way. (That integers are 2s complement representation is another.)
>
> Also very true.  I don't like seeing the code in question though.
> We've several implementations of sign-extend-to-N-bits functions
> throughout qemu; we ought to unify them.
>
>
> r~

Regards,
Jia.