Re: [Qemu-devel] [PATCH v6 03/13] target-mips-ase-dsp: Add branch instructions

[Aurelien Jarno <aurelien@aurel32.net>]

On Tue, Aug 21, 2012 at 02:53:09PM +0800, Jia Liu wrote:
> Add MIPS ASE DSP Branch instructions.
> 
> Signed-off-by: Jia Liu <proljc@gmail.com>
> ---
>  target-mips/translate.c |   52 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 0793153..b049238 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -332,6 +332,14 @@ enum {
>      OPC_DSHD     = (0x05 << 6) | OPC_DBSHFL,
>  };
>  
> +/* MIPS DSP REGIMM opcodes */
> +enum {
> +    OPC_BPOSGE32 = (0x1C << 16) | OPC_REGIMM,
> +#if defined(TARGET_MIPS64)
> +    OPC_BPOSGE64 = (0x1D << 16) | OPC_REGIMM,
> +#endif
> +};
> +
>  /* Coprocessor 0 (rs field) */
>  #define MASK_CP0(op)       MASK_OP_MAJOR(op) | (op & (0x1F << 21))
>  
> @@ -2798,6 +2806,24 @@ static void gen_compute_branch (DisasContext *ctx, uint32_t opc,
>          }
>          btgt = ctx->pc + insn_bytes + offset;
>          break;
> +    case OPC_BPOSGE32:
> +        tcg_gen_mov_tl(t0, cpu_dspctrl);
> +#if defined(TARGET_MIPS64)
> +        tcg_gen_andi_tl(t0, t0, 0x7F);
> +#else
> +        tcg_gen_andi_tl(t0, t0, 0x3F);
> +#endif

You can directly use cpu_dspctrl as the second argument of andi_tl. I
know this is optimized later by TCG, but it will still be faster if you
use less instructions.

> +        bcond_compute = 1;
> +        btgt = ctx->pc + insn_bytes + offset;
> +        break;
> +#if defined(TARGET_MIPS64)
> +    case OPC_BPOSGE64:
> +        tcg_gen_mov_tl(t0, cpu_dspctrl);
> +        tcg_gen_andi_tl(t0, t0, 0x7F);

Same here.

> +        bcond_compute = 1;
> +        btgt = ctx->pc + insn_bytes + offset;
> +        break;
> +#endif
>      case OPC_J:
>      case OPC_JAL:
>      case OPC_JALX:
> @@ -2986,6 +3012,16 @@ static void gen_compute_branch (DisasContext *ctx, uint32_t opc,
>              tcg_gen_setcondi_tl(TCG_COND_LT, bcond, t0, 0);
>              MIPS_DEBUG("bltzl %s, " TARGET_FMT_lx, regnames[rs], btgt);
>              goto likely;
> +        case OPC_BPOSGE32:
> +            tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 32);
> +            MIPS_DEBUG("bposge32 %s, " TARGET_FMT_lx, t0, btgt);
> +            goto not_likely;
> +#if defined(TARGET_MIPS64)
> +        case OPC_BPOSGE64:
> +            tcg_gen_setcondi_tl(TCG_COND_GE, bcond, t0, 64);
> +            MIPS_DEBUG("bposge64 %s, " TARGET_FMT_lx, t0, btgt);
> +            goto not_likely;
> +#endif
>          case OPC_BLTZALS:
>          case OPC_BLTZAL:
>              ctx->hflags |= (opc == OPC_BLTZALS
> @@ -11222,10 +11258,6 @@ static void decode_micromips32_opc (CPUMIPSState *env, DisasContext *ctx,
>                                  (ctx->opcode >> 18) & 0x7, imm << 1);
>              *is_branch = 1;
>              break;
> -        case BPOSGE64:
> -        case BPOSGE32:
> -            /* MIPS DSP: not implemented */
> -            /* Fall through */
>          default:
>              MIPS_INVAL("pool32i");
>              generate_exception(ctx, EXCP_RI);
> @@ -12134,6 +12166,18 @@ static void decode_opc (CPUMIPSState *env, DisasContext *ctx, int *is_branch)
>              check_insn(env, ctx, ISA_MIPS32R2);
>              /* Treat as NOP. */
>              break;
> +        case OPC_BPOSGE32:    /* MIPS DSP branch */
> +            check_insn(env, ctx, ASE_DSP);

check_insn() is not enough there. It only checks if the CPU support the
instruction set, while the DSP ASE manual says that the DSPEn bit should
be set to allow execution of DSP instructions, otherwise a State
Disabled Exception should be generated. It's something important so that
the OS can do lazy context switching.

Please look for example at the check_cop1x() instruction to see how to
do that.

> +            gen_compute_branch(ctx, op1, 4, -1, -2, (int32_t)imm << 2);
> +            *is_branch = 1;
> +            break;
> +#if defined(TARGET_MIPS64)
> +        case OPC_BPOSGE64:
> +            check_insn(env, ctx, ASE_DSP);
> +            gen_compute_branch(ctx, op1, 4, -1, -2, (int32_t)imm << 2);
> +            *is_branch = 1;
> +            break;
> +#endif
>          default:            /* Invalid */
>              MIPS_INVAL("regimm");
>              generate_exception(ctx, EXCP_RI);
> -- 
> 1.7.9.5
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net