Re: [Qemu-devel] [PATCH 3/4] target-mips:Support for Cavium specific instructions

[Richard Henderson]

>  }
> +#if defined(TARGET_MIPS64)
> +/* set on equal/not equal immidiate */

You need blank lines between all of these functions.
Also, "immediate" is misspelled.

> +    tcg_gen_xori_tl(t0, t0, uimm);
> +    switch (opc) {
> +    case OPC_SEQI:
> +        tcg_gen_setcondi_tl(TCG_COND_LT, cpu_gpr[rt], t0, 1);

For both gen_set_imm and gen_set, you're thinking about these
operations how you'd implement them with mips native insns.
For TCG this should be

  tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_gpr[rt], cpu_gpr[rs], uimm);


> +/* Store atomic add */
> +/* FIXME: something else should be done for emulating SMP system. */

See how EXCP_SC is used here in the translator and in linux-user/main.c.

> +/* Cavium specific extract instructions */
> +static void gen_exts(CPUState *env, DisasContext *ctx, uint32_t opc, int rt,
> +                      int rs, int lsb, int msb)
> +{
> +    TCGv t0 = tcg_temp_new();
> +    TCGv t1 = tcg_temp_new();
> +    target_ulong mask;
> +    gen_load_gpr(t1, rs);
> +    switch (opc) {
> +    case OPC_EXTS:
> +        tcg_gen_shri_tl(t0, t1, lsb);
> +        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> +        /* To sign extened the remaining bits according to
> +           the msb of the bit field */
> +        mask = 1ULL << msb;
> +        tcg_gen_andi_tl(t1, t0, mask);
> +        tcg_gen_addi_tl(t1, t1, -1);
> +        tcg_gen_orc_tl(t0, t0, t1);
> +        gen_store_gpr(t0, rt);

This can be implemented with exactly two shifts:

    lshift = 64 - msb - 1;
    rshift = lshift + lsb;

    tcg_gen_shli_tl(t0, t0, lshift);
    tcg_gen_sari_tl(to, to, rshift);
    

>      OPC_MUL      = 0x02 | OPC_SPECIAL2,
> +    /* Cavium Specific Instructions */
> +    OPC_BADDU    = 0x28 | OPC_SPECIAL2,
> +    OPC_DMUL     = 0x03 | OPC_SPECIAL2,
> +    OPC_EXTS     = 0x3a | OPC_SPECIAL2,
> +    OPC_EXTS32   = 0x3b | OPC_SPECIAL2,
> +    OPC_CINS     = 0x32 | OPC_SPECIAL2,
> +    OPC_CINS32   = 0x33 | OPC_SPECIAL2,
> +    OPC_SEQI     = 0x2e | OPC_SPECIAL2,
> +    OPC_SNEI     = 0x2f | OPC_SPECIAL2,
> +    OPC_MTM0     = 0x08 | OPC_SPECIAL2,
> +    OPC_MTM1     = 0x0c | OPC_SPECIAL2,
> +    OPC_MTM2     = 0x0d | OPC_SPECIAL2,
> +    OPC_MTP0     = 0x09 | OPC_SPECIAL2,
> +    OPC_MTP1     = 0x0a | OPC_SPECIAL2,
> +    OPC_MTP2     = 0x0b | OPC_SPECIAL2,
> +    OPC_V3MULU   = 0x11 | OPC_SPECIAL2,
> +    OPC_VMM0     = 0x10 | OPC_SPECIAL2,
> +    OPC_VMULU    = 0x0f | OPC_SPECIAL2,
> +    OPC_POP      = 0X2C | OPC_SPECIAL2,
> +    OPC_DPOP     = 0X2D | OPC_SPECIAL2,
> +    OPC_SEQ      = 0x2a | OPC_SPECIAL2,
> +    OPC_SNE      = 0x2b | OPC_SPECIAL2,
> +    OPC_SAA      = 0x18 | OPC_SPECIAL2,
> +    OPC_SAAD     = 0x19 | OPC_SPECIAL2,
> +/**************************************/
>      OPC_MSUB     = 0x04 | OPC_SPECIAL2,
>      OPC_MSUBU    = 0x05 | OPC_SPECIAL2,
>      /* Loongson 2F */

You'll notice that there are already nicely separated sections
there in the enums for SPECIAL2, and you've dropped yours right
in the middle.  Don't do that.  Nor the ***** thing.


r~