Re: [Qemu-devel] [PATCH 1/4] target-tricore: target-tricore: Add instructions of RR1 opcode format, that have 0x93 as first opcode

[Richard Henderson <rth@twiddle.net>]

On 01/21/2015 10:04 AM, Bastian Koppelmann wrote:
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> ---
>  target-tricore/translate.c | 276 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 276 insertions(+)
> 
> diff --git a/target-tricore/translate.c b/target-tricore/translate.c
> index def7f4a..da8ecbc 100644
> --- a/target-tricore/translate.c
> +++ b/target-tricore/translate.c
> @@ -4778,6 +4778,279 @@ static void decode_rr1_mul(CPUTriCoreState *env, DisasContext *ctx)
>      tcg_temp_free(n);
>  }
>  
> +static void decode_rr1_mulq(CPUTriCoreState *env, DisasContext *ctx)
> +{
> +    uint32_t op2;
> +    int r1, r2, r3;
> +    uint32_t n;
> +
> +    TCGv temp, temp2;
> +
> +    r1 = MASK_OP_RR1_S1(ctx->opcode);
> +    r2 = MASK_OP_RR1_S2(ctx->opcode);
> +    r3 = MASK_OP_RR1_D(ctx->opcode);
> +    n  = MASK_OP_RR1_N(ctx->opcode);
> +    op2 = MASK_OP_RR1_OP2(ctx->opcode);
> +
> +    temp = tcg_temp_new();
> +    temp2 = tcg_temp_new();
> +
> +    switch (op2) {
> +    case OPC2_32_RR1_MUL_Q_32:
> +        if (n == 0) {
> +            tcg_gen_muls2_tl(temp, cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2]);
> +            /* reset v bit */
> +            tcg_gen_movi_tl(cpu_PSW_V, 0);
> +        } else {
> +            tcg_gen_muls2_tl(temp, temp2, cpu_gpr_d[r1], cpu_gpr_d[r2]);
> +            tcg_gen_shli_tl(temp2, temp2, n);
> +            tcg_gen_shri_tl(temp, temp, 31);

Yes, n is supposed to be either 0 or 1.
But mixing n with a constant is confusing.

Either hard-code 1 here (perhaps preferred?),
or write 32-n.

> +    case OPC2_32_RR1_MUL_Q_64:
> +        if (n == 0) {
> +            tcg_gen_muls2_tl(cpu_gpr_d[r3], cpu_gpr_d[r3+1], cpu_gpr_d[r1],
> +                             cpu_gpr_d[r2]);
> +            /* reset v bit */
> +            tcg_gen_movi_tl(cpu_PSW_V, 0);
> +        } else {
> +            tcg_gen_muls2_tl(temp, temp2, cpu_gpr_d[r1], cpu_gpr_d[r2]);
> +            tcg_gen_shli_tl(temp2, temp2, n);
> +            tcg_gen_shli_tl(cpu_gpr_d[r3], temp, n);
> +            tcg_gen_shri_tl(temp, temp, 31);
> +            tcg_gen_or_tl(cpu_gpr_d[r3+1], temp, temp2);

I do wonder about just using 64-bit arithmetic here, instead of
emulating a 64-bit shift.

> +            /* overflow only occours if r1 = r2 = 0x8000 */
> +            tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_PSW_V, cpu_gpr_d[r3+1],
> +                                0x80000000);
> +            tcg_gen_shli_tl(cpu_PSW_V, cpu_PSW_V, 31);
> +        }
> +        /* calc sv overflow bit */
> +        tcg_gen_or_tl(cpu_PSW_SV, cpu_PSW_SV, cpu_PSW_V);
> +        /* calc av overflow bit */
> +        tcg_gen_add_tl(cpu_PSW_AV, cpu_gpr_d[r3+1], cpu_gpr_d[r3+1]);
> +        tcg_gen_xor_tl(cpu_PSW_AV, cpu_gpr_d[r3+1], cpu_PSW_AV);
> +        /* calc sav overflow bit */
> +        tcg_gen_or_tl(cpu_PSW_SAV, cpu_PSW_SAV, cpu_PSW_AV);
> +        break;
> +    case OPC2_32_RR1_MUL_Q_32_L:
> +        tcg_gen_ext16s_tl(temp, cpu_gpr_d[r2]);
> +        if (n == 0) {
> +            tcg_gen_muls2_tl(temp, temp2, temp, cpu_gpr_d[r1]);
> +            tcg_gen_shli_tl(cpu_gpr_d[r3], temp2, 16);
> +            tcg_gen_shri_tl(temp, temp, 16);
> +            tcg_gen_or_tl(cpu_gpr_d[r3], cpu_gpr_d[r3], temp);

Similarly.

> +            /* reset v bit */
> +            tcg_gen_movi_tl(cpu_PSW_V, 0);
> +        } else {
> +            tcg_gen_muls2_tl(temp, temp2, temp, cpu_gpr_d[r1]);
> +            tcg_gen_shli_tl(cpu_gpr_d[r3], temp2, 17);
> +            tcg_gen_shri_tl(temp, temp, 15);
> +            tcg_gen_or_tl(cpu_gpr_d[r3], cpu_gpr_d[r3], temp);
> +            /* overflow only occours if r1 = r2 = 0x8000 */
> +            tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_PSW_V, cpu_gpr_d[r3],
> +                                0x80000000);
> +            tcg_gen_shli_tl(cpu_PSW_V, cpu_PSW_V, 31);
> +        }
> +        /* calc sv overflow bit */
> +        tcg_gen_or_tl(cpu_PSW_SV, cpu_PSW_SV, cpu_PSW_V);
> +        /* calc av overflow bit */
> +        tcg_gen_add_tl(cpu_PSW_AV, cpu_gpr_d[r3], cpu_gpr_d[r3]);
> +        tcg_gen_xor_tl(cpu_PSW_AV, cpu_gpr_d[r3], cpu_PSW_AV);
> +        /* calc sav overflow bit */
> +        tcg_gen_or_tl(cpu_PSW_SAV, cpu_PSW_SAV, cpu_PSW_AV);
> +        break;
> +    case OPC2_32_RR1_MUL_Q_64_L:
> +        tcg_gen_ext16s_tl(temp, cpu_gpr_d[r2]);
> +        if (n == 0) {
> +            tcg_gen_muls2_tl(cpu_gpr_d[r3], cpu_gpr_d[r3+1], cpu_gpr_d[r1],
> +                             temp);
> +            /* reset v bit */
> +            tcg_gen_movi_tl(cpu_PSW_V, 0);
> +        } else {
> +            tcg_gen_muls2_tl(temp, temp2, cpu_gpr_d[r1], temp);
> +            tcg_gen_shli_tl(temp2, temp2, n);
> +            tcg_gen_shli_tl(cpu_gpr_d[r3], temp, n);
> +            tcg_gen_shri_tl(temp, temp, 31);
> +            tcg_gen_or_tl(cpu_gpr_d[r3+1], temp, temp2);
> +            /* overflow only occours if r1 = r2 = 0x8000 */
> +            tcg_gen_setcondi_tl(TCG_COND_EQ, cpu_PSW_V, cpu_gpr_d[r3+1],
> +                                0x80000000);
> +            tcg_gen_shli_tl(cpu_PSW_V, cpu_PSW_V, 31);
> +        }
> +        /* calc sv overflow bit */
> +        tcg_gen_or_tl(cpu_PSW_SV, cpu_PSW_SV, cpu_PSW_V);
> +        /* calc av overflow bit */
> +        tcg_gen_add_tl(cpu_PSW_AV, cpu_gpr_d[r3+1], cpu_gpr_d[r3+1]);
> +        tcg_gen_xor_tl(cpu_PSW_AV, cpu_gpr_d[r3+1], cpu_PSW_AV);
> +        /* calc sav overflow bit */
> +        tcg_gen_or_tl(cpu_PSW_SAV, cpu_PSW_SAV, cpu_PSW_AV);
> +        break;
> +    case OPC2_32_RR1_MUL_Q_32_U:
> +        tcg_gen_shri_tl(temp, cpu_gpr_d[r2], 16);
> +        tcg_gen_ext16s_tl(temp, temp);

Use an arithmetic shift and you don't need the sign-extend.

There's an awful lot of replication in here.  I think a few
different subroutines are warrented.


r~