Re: [Qemu-devel] [PATCH 10/15] target-tricore: Add instructions of SB opcode format

[Richard Henderson <rth@twiddle.net>]

On 07/07/2014 11:13 AM, Bastian Koppelmann wrote:
> diff --git a/target-tricore/op_helper.c b/target-tricore/op_helper.c
> index b9fbfad..5267fd0 100644
> --- a/target-tricore/op_helper.c
> +++ b/target-tricore/op_helper.c
> @@ -56,6 +56,198 @@ target_ulong helper_shac(CPUTRICOREState *env, target_ulong r1,
>      return ret;
>  }
>  
> +/* context save area (CSA) related helpers */
> +
> +enum {
> +    CONTEXT_LOWER = 0,
> +    CONTEXT_UPPER = 1,
> +};
> +
> +static int cdc_increment(TCState *tc)
> +{
> +    tc->PSW++;
> +    return 0;
> +}
> +
> +static int cdc_decrement(TCState *tc)
> +{
> +    tc->PSW--;
> +    return 0;
> +}

I guess these will be filled in more later?

> +static void save_context(CPUTRICOREState *env, int ea, int ul,
> +                         target_ulong *new_FCX)
> +{
> +    *new_FCX = cpu_ldl_data(env, ea);
> +    cpu_stl_data(env, ea, env->active_tc.PCXI);
> +    if (ul == CONTEXT_UPPER) {
> +        cpu_stl_data(env, ea+4, env->active_tc.PSW);
> +        cpu_stl_data(env, ea+8, env->active_tc.gpr_a[10]);
> +        cpu_stl_data(env, ea+12, env->active_tc.gpr_a[11]);
> +        cpu_stl_data(env, ea+16, env->active_tc.gpr_d[8]);
> +        cpu_stl_data(env, ea+20, env->active_tc.gpr_d[9]);
> +        cpu_stl_data(env, ea+24, env->active_tc.gpr_d[10]);
> +        cpu_stl_data(env, ea+28, env->active_tc.gpr_d[11]);
> +        cpu_stl_data(env, ea+32, env->active_tc.gpr_a[12]);
> +        cpu_stl_data(env, ea+36, env->active_tc.gpr_a[13]);
> +        cpu_stl_data(env, ea+40, env->active_tc.gpr_a[14]);
> +        cpu_stl_data(env, ea+44, env->active_tc.gpr_a[15]);
> +        cpu_stl_data(env, ea+48, env->active_tc.gpr_d[12]);
> +        cpu_stl_data(env, ea+52, env->active_tc.gpr_d[13]);
> +        cpu_stl_data(env, ea+56, env->active_tc.gpr_d[14]);
> +        cpu_stl_data(env, ea+60, env->active_tc.gpr_d[15]);

I wonder if this ought to be split into separate functions.  Certainly you know
for a call that it's going to be the upper context, so why test?

> +static inline void gen_branch_cond(DisasContext *ctx, int cond, TCGv r1,
> +                                   TCGv r2, int16_t address)
> +{
> +    int jumpLabel;
> +    jumpLabel = gen_new_label();
> +    tcg_gen_brcond_tl(cond, r1, r2, jumpLabel);
> +
> +    gen_save_pc(ctx->pc + insn_bytes);

insn_bytes should be part of ctx.  Alternately, pre-compute ctx->next_pc =
ctx->pc + insn_bytes, since that appears to be more useful.

> +    tcg_gen_exit_tb(0);
> +
> +    gen_set_label(jumpLabel);
> +    gen_save_pc(ctx->pc + address * 2);
> +    tcg_gen_exit_tb(0);
> +
> +}

Watch the useless blank lines.

You'll want to emit goto_tb opcodes when appropriate.  See examples in other
targets for when this is possible.

> +    case OPC1_16_SB_CALL:
> +        gen_helper_2arg(call, ctx->pc, insn_bytes);
> +        gen_save_pc(ctx->pc+sign_extend(offset, 7)*2);
> +        tcg_gen_exit_tb(0);

Why pass pc and insn_bytes to helper_call, when all that happens inside is that
they're added together?  Or for that matter why pass either at all since you
could just as well emit

  tcg_gen_movi_tl(cpu_gen_a[11], ctx->pc + insn_bytes);

And again you'll want to emit goto_tb if possible.

> +/* SB-format */
> +    case OPC1_16_SB_CALL:
> +        address = MASK_OP_SB_DISP8(ctx->opcode);
> +        gen_compute_branch(ctx, op1, 0, 0, 0, address);
> +        break;
> +    case OPC1_16_SB_J:
> +        address = MASK_OP_SB_DISP8(ctx->opcode);
> +        gen_compute_branch(ctx, op1, 0, 0, 0, address);
> +        break;
> +    case OPC1_16_SB_JNZ:
> +        address = sign_extend(MASK_OP_SB_DISP8(ctx->opcode), 7);
> +        gen_compute_branch(ctx, op1, 0, 0, 0, address);
> +        break;
> +    case OPC1_16_SB_JZ:
> +        address = sign_extend(MASK_OP_SB_DISP8(ctx->opcode), 7);
> +        gen_compute_branch(ctx, op1, 0, 0, 0, address);

Surely all these should be one common call to gen_compute_branch.


r~