Re: [PATCH 05/27] arc: TCG instruction generator and hand-definitions

[Richard Henderson]

On 4/5/21 7:31 AM, cupertinomiranda@gmail.com wrote:
> +/*
> + * Function to add boiler plate code for conditional execution.
> + * It will add tcg_gen codes only if there is a condition to
> + * be checked (ctx->insn.cc != 0).
> + * Remember to pair it with gen_cc_epilogue(ctx) macro.
> + */
> +static void gen_cc_prologue(DisasCtxt *ctx)
> +{
> +    ctx->tmp_reg = tcg_temp_local_new();
> +    ctx->label = gen_new_label();

There's no point in creating these when insn.cc is false.

> +    if (ctx->insn.cc) {
> +        arc_gen_verifyCCFlag(ctx, ctx->tmp_reg);
> +        tcg_gen_brcondi_tl(TCG_COND_NE, ctx->tmp_reg, 1, ctx->label);

You don't need a local temp for tmp_reg.
You can create and free it here.

> +/*
> + * The finishing counter part of gen_cc_prologue. This is supposed
> + * to be put at the end of the function using it.
> + */
> +static void gen_cc_epilogue(const DisasCtxt *ctx)
> +{
> +    if (ctx->insn.cc) {
> +        gen_set_label(ctx->label);
> +    }
> +    tcg_temp_free(ctx->tmp_reg);
> +}

You do not need to keep it live til here.

> +/*
> + * Populates a 32-bit vector with repeating SHIMM:
> + *   vec32=(0000000000u6,0000000000u6)
> + *   vec32=(sssss12,sssss12)
> + * It's crucial that the s12 part of an encoding is in signed
> + * integer form while passed along in SHIMM, e.g:
> + *   s12 = -125 (0xf803) --> 0xfffff803
> + * Do not forget to free the returned TCGv_i32 when done!
> + */
> +static TCGv_i32 dup_shimm_to_i32(int16_t shimm)
> +{
> +    TCGv_i32 vec32 = tcg_temp_new_i32();
> +    int32_t val = shimm;
> +    val = ((val << 16) & 0xffff0000) | (val & 0xffff);
> +    tcg_gen_movi_i32(vec32, val);
> +    return vec32;
> +}

  return tcg_constant_i32(dup_const(MO_16, shimm));

> +static TCGv_i64 dup_limm_to_i64(int32_t limm)
> +{
> +    TCGv_i64 vec64 = tcg_temp_new_i64();
> +    int64_t val = limm;
> +    val = (val << 32) | (val & 0xffffffff);
> +    tcg_gen_movi_i64(vec64, val);
> +    return vec64;
> +}

  return tcg_constant_i64(dup_const(MO_32, limm));

> +static TCGv_i64 quad_shimm_to_i64(int16_t shimm)
> +{
> +    TCGv_i64 vec64 = tcg_temp_new_i64();
> +    int64_t val = shimm;
> +    val = (val << 48) | ((val << 32) & 0x0000ffff00000000) |
> +          ((val << 16) & 0x00000000ffff0000) | (val & 0xffff);
> +    tcg_gen_movi_i64(vec64, val);
> +    return vec64;
> +}

  return tcg_constant_i64(dup_const(MO_16, shimm));

Note that, tcg_constant_* are hashed and need not be freed (but are silently 
accepted if you do).  However, you cannot assign to them.  If you really 
require a mutable temporary, use tcg_const_*.


> +static void gen_vec_op2h(const DisasCtxt *ctx,
> +                         void (*OP)(TCGv, TCGv, TCGv),
> +                         TCGv_i32 dest,
> +                         TCGv_i32 b32,
> +                         TCGv_i32 c32)
> +{
> +    TCGv_i32 t0, t1;
> +
> +    /* If no real register for result, then this a nop. Bail out! */
> +    if (!(ctx->insn.operands[0].type & ARC_OPERAND_IR)) {
> +        return;
> +    }
> +
> +    t0 = tcg_temp_new();
> +    tcg_gen_mov_i32(t0, b32);
> +    /*
> +     * If the last operand is a u6/s12, say 63, there is no "HI" in it.
> +     * Instead, it must be duplicated to form a pair; e.g.: (63, 63).
> +     */
> +    if (ctx->insn.operands[2].type & ARC_OPERAND_SHIMM) {
> +        t1 = dup_shimm_to_i32(ctx->insn.operands[2].value);
> +    } else {
> +        t1 = tcg_temp_new();
> +        tcg_gen_mov_i32(t1, c32);
> +    }
> +
> +    (*OP)(dest, t0, t1);
> +
> +    tcg_temp_free(t1);
> +    tcg_temp_free(t0);
> +}

Why are you copying b32 to a new temp?  Do you have callbacks that are not 
careful about overlap between dest, t0, t1, and assume that you have 
non-overlapping inputs?

If you don't require uniqueness, and use tcg_constant_* above, then you don't 
need to copy c32 into a new temp either, nor worry about conditionally freeing it.

> +static void gen_add16(TCGv_i32 dest, TCGv_i32 t0, TCGv_i32 t1)
> +{
> +    TCGv_i32 tmp = tcg_temp_new_i32();
> +    tcg_gen_xor_i32(tmp, t0, t1);
> +    tcg_gen_andi_i32(tmp, tmp, 0x8000);
> +    tcg_gen_andi_i32(t0, t0, ~0x8000);
> +    tcg_gen_andi_i32(t1, t1, ~0x8000);
> +    tcg_gen_add_i32(t0, t0, t1);
> +    tcg_gen_xor_i32(dest, t0, tmp);
> +    tcg_temp_free_i32(tmp);
> +}

I guess that answers my question of whether you're relying on new unique temps 
as inputs: yes.

> +static void arc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> +{
> +    bool in_a_delayslot_instruction = false;
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
> +    CPUARCState *env = cpu->env_ptr;
> +
> +    /* TODO (issue #62): these must be removed */
> +    dc->zero = tcg_const_local_tl(0);
> +    dc->one  = tcg_const_local_tl(1);
> +
> +    if (env->stat.is_delay_slot_instruction == 1) {
> +        in_a_delayslot_instruction = true;
> +    }
> +
> +    dc->cpc = dc->base.pc_next;
> +    decode_opc(env, dc);
> +
> +    dc->base.pc_next = dc->npc;
> +    tcg_gen_movi_tl(cpu_npc, dc->npc);
> +
> +    if (in_a_delayslot_instruction == true) {
> +        dc->base.is_jmp = DISAS_NORETURN;
> +
> +        /* Post execution delayslot logic. */
> +        TCGLabel *DEf_not_set_label1 = gen_new_label();
> +        tcg_gen_brcondi_tl(TCG_COND_NE, cpu_DEf, 1, DEf_not_set_label1);
> +        tcg_gen_movi_tl(cpu_DEf, 0);
> +        gen_goto_tb(dc, 1, cpu_bta);
> +        gen_set_label(DEf_not_set_label1);
> +        env->stat.is_delay_slot_instruction = 0;

You may not examine or modify env directly during translate.
This absolutely will not work.


r~