Re: [Qemu-devel] [PATCH v2 11/20] target-mips: add MSA I5 format instruction

[Leon Alrae <leon.alrae@imgtec.com>]

On 29/10/14 01:41, Yongbok Kim wrote:
> +    uint8_t df = (ctx->opcode >> 21) & 0x3;
> +    int64_t s5 = (ctx->opcode >> 16) & 0x1f;
> +    s5 = (s5 << 59) >> 59; /* sign extend s5 to 64 bits*/

Mixed declarations and code are not allowed. This issue occurs also in
subsequent patches (12, 15, 17, 18) in this series. You may also
consider using sextract() for s5.

> +    uint8_t u5 = (ctx->opcode >> 16) & 0x1f;
> +    uint8_t ws = (ctx->opcode >> 11) & 0x1f;
> +    uint8_t wd = (ctx->opcode >> 6) & 0x1f;
> +
> +    TCGv_i32 tdf = tcg_const_i32(df);
> +    TCGv_i32 twd = tcg_const_i32(wd);
> +    TCGv_i32 tws = tcg_const_i32(ws);
> +    TCGv_i64 tu5 = tcg_const_i64(u5);
> +    TCGv_i64 ts5 = tcg_const_i64(s5);

One of above tcg variable is redundant as tu5 and ts5 are never used
together. Have you considered to have just one tcg variable initialized
later - in case blocks - with appropriate value? You already did this
for ts10 in case OPC_LDI_df.

> +
> +    switch (MASK_MSA_I5(opcode)) {
> +    case OPC_ADDVI_df:
> +        gen_helper_msa_addvi_df(cpu_env, tdf, twd, tws, tu5);
> +        break;
> +    case OPC_SUBVI_df:
> +        gen_helper_msa_subvi_df(cpu_env, tdf, twd, tws, tu5);
> +        break;
> +    case OPC_MAXI_S_df:
> +        gen_helper_msa_maxi_s_df(cpu_env, tdf, twd, tws, ts5);
> +        break;
> +    case OPC_MAXI_U_df:
> +        gen_helper_msa_maxi_u_df(cpu_env, tdf, twd, tws, tu5);
> +        break;
> +    case OPC_MINI_S_df:
> +        gen_helper_msa_mini_s_df(cpu_env, tdf, twd, tws, ts5);
> +        break;
> +    case OPC_MINI_U_df:
> +        gen_helper_msa_mini_u_df(cpu_env, tdf, twd, tws, tu5);
> +        break;
> +    case OPC_CEQI_df:
> +        gen_helper_msa_ceqi_df(cpu_env, tdf, twd, tws, ts5);
> +        break;
> +    case OPC_CLTI_S_df:
> +        gen_helper_msa_clti_s_df(cpu_env, tdf, twd, tws, ts5);
> +        break;
> +    case OPC_CLTI_U_df:
> +        gen_helper_msa_clti_u_df(cpu_env, tdf, twd, tws, tu5);
> +        break;
> +    case OPC_CLEI_S_df:
> +        gen_helper_msa_clei_s_df(cpu_env, tdf, twd, tws, ts5);
> +        break;
> +    case OPC_CLEI_U_df:
> +        gen_helper_msa_clei_u_df(cpu_env, tdf, twd, tws, tu5);
> +        break;
> +    case OPC_LDI_df:
> +        {
> +            int64_t s10 = (ctx->opcode >> 11) & 0x3ff;
> +            s10 = (s10 << 54) >> 54; /* sign extend s10 to 64 bits*/

Mixed declarations and code

> +
> +            TCGv_i32 ts10 = tcg_const_i32(s10);
> +            gen_helper_msa_ldi_df(cpu_env, tdf, twd, ts10);
> +            tcg_temp_free_i32(ts10);