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

[Richard Henderson]

On 11/11/20 10:17 AM, cupertinomiranda@gmail.com wrote:
> +/*
> + * The macro 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). This macro assumes that there
> + * is a "ctx" variable of type "DisasCtxt *" in context. Remember
> + * to pair it with CC_EPILOGUE macro.
> + */
> +#define CC_PROLOGUE                                   \
> +  TCGv cc = tcg_temp_local_new();                     \
> +  TCGLabel *done = gen_new_label();                   \
> +  do {                                                \
> +    if (ctx->insn.cc) {                               \
> +        arc_gen_verifyCCFlag(ctx, cc);                \
> +        tcg_gen_brcondi_tl(TCG_COND_NE, cc, 1, done); \
> +    }                                                 \
> +  } while (0)
> +
> +/*
> + * The finishing counter part of CC_PROLUGE. This is supposed
> + * to be put at the end of the function using it.
> + */
> +#define CC_EPILOGUE          \
> +    if (ctx->insn.cc) {      \
> +        gen_set_label(done); \
> +    }                        \
> +    tcg_temp_free(cc)

Why would this need to be boiler-plate?  Why would this not simply exist in
exactly one location?

You don't need a tcg_temp_local_new, because the cc value is not used past the
branch.  You should free the temp immediately after the branch.

> +void gen_goto_tb(DisasContext *ctx, int n, TCGv dest)
> +{
> +    tcg_gen_mov_tl(cpu_pc, dest);
> +    tcg_gen_andi_tl(cpu_pcl, dest, 0xfffffffc);
> +    if (ctx->base.singlestep_enabled) {
> +        gen_helper_debug(cpu_env);
> +    }
> +    tcg_gen_exit_tb(NULL, 0);

Missing else.  This is dead code for single-step.

> +void arc_translate_init(void)
> +{
> +    int i;
> +    static int init_not_done = 1;
> +
> +    if (init_not_done == 0) {
> +        return;
> +    }

Useless.  This will only be called once.

> +#define ARC_REG_OFFS(x) offsetof(CPUARCState, x)
> +
> +#define NEW_ARC_REG(x) \
> +        tcg_global_mem_new_i32(cpu_env, offsetof(CPUARCState, x), #x)
> +
> +    cpu_S1f = NEW_ARC_REG(macmod.S1);
> +    cpu_S2f = NEW_ARC_REG(macmod.S2);
> +    cpu_CSf = NEW_ARC_REG(macmod.CS);
> +
> +    cpu_Zf  = NEW_ARC_REG(stat.Zf);
> +    cpu_Lf  = NEW_ARC_REG(stat.Lf);
> +    cpu_Nf  = NEW_ARC_REG(stat.Nf);
> +    cpu_Cf  = NEW_ARC_REG(stat.Cf);
> +    cpu_Vf  = NEW_ARC_REG(stat.Vf);
> +    cpu_Uf  = NEW_ARC_REG(stat.Uf);
> +    cpu_DEf = NEW_ARC_REG(stat.DEf);
> +    cpu_ESf = NEW_ARC_REG(stat.ESf);
> +    cpu_AEf = NEW_ARC_REG(stat.AEf);
> +    cpu_Hf  = NEW_ARC_REG(stat.Hf);
> +    cpu_IEf = NEW_ARC_REG(stat.IEf);
> +    cpu_Ef  = NEW_ARC_REG(stat.Ef);
> +
> +    cpu_is_delay_slot_instruction = 
> NEW_ARC_REG(stat.is_delay_slot_instruction);
> +
> +    cpu_l1_Zf = NEW_ARC_REG(stat_l1.Zf);
> +    cpu_l1_Lf = NEW_ARC_REG(stat_l1.Lf);
> +    cpu_l1_Nf = NEW_ARC_REG(stat_l1.Nf);
> +    cpu_l1_Cf = NEW_ARC_REG(stat_l1.Cf);
> +    cpu_l1_Vf = NEW_ARC_REG(stat_l1.Vf);
> +    cpu_l1_Uf = NEW_ARC_REG(stat_l1.Uf);
> +    cpu_l1_DEf = NEW_ARC_REG(stat_l1.DEf);
> +    cpu_l1_AEf = NEW_ARC_REG(stat_l1.AEf);
> +    cpu_l1_Hf = NEW_ARC_REG(stat_l1.Hf);
> +
> +    cpu_er_Zf = NEW_ARC_REG(stat_er.Zf);
> +    cpu_er_Lf = NEW_ARC_REG(stat_er.Lf);
> +    cpu_er_Nf = NEW_ARC_REG(stat_er.Nf);
> +    cpu_er_Cf = NEW_ARC_REG(stat_er.Cf);
> +    cpu_er_Vf = NEW_ARC_REG(stat_er.Vf);
> +    cpu_er_Uf = NEW_ARC_REG(stat_er.Uf);
> +    cpu_er_DEf = NEW_ARC_REG(stat_er.DEf);
> +    cpu_er_AEf = NEW_ARC_REG(stat_er.AEf);
> +    cpu_er_Hf = NEW_ARC_REG(stat_er.Hf);
> +
> +    cpu_eret = NEW_ARC_REG(eret);
> +    cpu_erbta = NEW_ARC_REG(erbta);
> +    cpu_ecr = NEW_ARC_REG(ecr);
> +    cpu_efa = NEW_ARC_REG(efa);
> +    cpu_bta = NEW_ARC_REG(bta);
> +    cpu_lps = NEW_ARC_REG(lps);
> +    cpu_lpe = NEW_ARC_REG(lpe);
> +    cpu_pc = NEW_ARC_REG(pc);
> +    cpu_npc = NEW_ARC_REG(npc);
> +
> +    cpu_bta_l1 = NEW_ARC_REG(bta_l1);
> +    cpu_bta_l2 = NEW_ARC_REG(bta_l2);
> +
> +    cpu_intvec = NEW_ARC_REG(intvec);
> +
> +    for (i = 0; i < 64; i++) {
> +        char name[16];
> +
> +        sprintf(name, "r[%d]", i);
> +
> +        cpu_r[i] = tcg_global_mem_new_i32(cpu_env,
> +                                          ARC_REG_OFFS(r[i]),
> +                                          strdup(name));
> +    }
> +
> +    cpu_gp     = cpu_r[26];
> +    cpu_fp     = cpu_r[27];
> +    cpu_sp     = cpu_r[28];
> +    cpu_ilink1 = cpu_r[29];
> +    cpu_ilink2 = cpu_r[30];
> +    cpu_blink  = cpu_r[31];
> +    cpu_acclo  = cpu_r[58];
> +    cpu_acchi  = cpu_r[59];
> +    cpu_lpc    = cpu_r[60];
> +    cpu_limm   = cpu_r[62];
> +    cpu_pcl    = cpu_r[63];

You shouldn't need two pointers to these.  Either use cpu_r[PCL] (preferred) or
#define cpu_pcl cpu_r[63].

You could put all of these into a const static table.

> +static int arc_gen_INVALID(const DisasContext *ctx)
> +{
> +    fprintf(stderr, "invalid inst @:%08x\n", ctx->cpc);

Never fprintf.  Raise an exception like you're supposed to.

> +/* Arrange to middle endian, used by LITTLE ENDIAN systems. */
> +static uint32_t arc_getm32(uint32_t data)
> +{
> +    uint32_t value = 0;
> +
> +    value  = (data & 0x0000ffff) << 16;
> +    value |= (data & 0xffff0000) >> 16;
> +    return value;
> +}

This is ror32(data, 16).

> +/* Check if OPR is a register _and_ an even numbered one. */
> +static inline bool is_odd_numbered_register(const operand_t opr)

comment s/even/odd/.

> +static void add_constant_operand(enum arc_opcode_map mapping,
> +                                 uint8_t operand_number,
> +                                 uint32_t value)
> +{
> +    struct constant_operands **t = &(map_constant_operands[mapping]);
> +    while (*t != NULL) {
> +        t = &((*t)->next);
> +    }
> +    *t = (struct constant_operands *) malloc(sizeof(struct 
> constant_operands));

Use g_new, which doesn't require error checking, which is missing here.

That said...

> +static void init_constants(void)
> +{
> +#define SEMANTIC_FUNCTION(...)
> +#define MAPPING(...)
> +#define CONSTANT(NAME, MNEMONIC, OP_NUM, VALUE) \
> +  add_constant_operand(MAP_##MNEMONIC##_##NAME, OP_NUM, VALUE);
> +#include "target/arc/semfunc_mapping.def"
> +#include "target/arc/extra_mapping.def"
> +#undef MAPPING
> +#undef CONSTANT
> +#undef SEMANTIC_FUNCTION
> +}

Ew.  Yet another thing that can be done at build time.

> +static TCGv arc_decode_operand(const struct arc_opcode *opcode,
> +                               DisasContext *ctx,
> +                               unsigned char nop,
> +                               enum arc_opcode_map mapping)
> +{
> +    TCGv ret;
> +
> +    if (nop >= ctx->insn.n_ops) {
> +        struct constant_operands *co = constant_entry_for(mapping, nop);
> +        assert(co != NULL);
> +        ret = tcg_const_local_i32(co->default_value);
> +        return ret;
> +    } else {
> +        operand_t operand = ctx->insn.operands[nop];
> +
> +        if (operand.type & ARC_OPERAND_IR) {
> +            ret = cpu_r[operand.value];
> +            if (operand.value == 63) {
> +                tcg_gen_movi_tl(cpu_pcl, ctx->pcl);
> +            }
> +      } else {

Really bad indention.

> +            int32_t limm = operand.value;
> +            if (operand.type & ARC_OPERAND_LIMM) {
> +                limm = ctx->insn.limm;
> +                tcg_gen_movi_tl(cpu_limm, limm);
> +                ret = cpu_r[62];
> +            } else {
> +                ret = tcg_const_local_i32(limm);
> +            }
> +        }
> +    }
> +
> +  return ret;

Why are you using locals for everything?  Is it because you have no proper
control over your use of branching?

> +    qemu_log_mask(CPU_LOG_TB_IN_ASM,
> +                  "CPU in sleep mode, waiting for an IRQ.\n");

Incorrect level at which to log this.

You wanted the logging at runtime, not translate. Which suggests you'd be
better off moving this entire function to a helper.

> +/* Return from exception. */
> +static void gen_rtie(DisasContext *ctx)
> +{
> +    tcg_gen_movi_tl(cpu_pc, ctx->cpc);
> +    gen_helper_rtie(cpu_env);
> +    tcg_gen_mov_tl(cpu_pc, cpu_pcl);
> +    gen_goto_tb(ctx, 1, cpu_pc);
> +}

You must return to the main loop here, not goto_tb.  You must return to the
main loop every time your interrupt mask changes, so that pending interrupts
may be accepted.

> +    val = ((val << 16) & 0xffff0000) | (val & 0xffff);

  val = deposit32(val, 16, 16, val);

> +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);

  val = deposit64(val, 32, 32, val);
or
  val = dup_const(val, MO_32);

> +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);

  val = dup_const(val, MO_16);

> + *    This is a truth table for XNOR(a,b):
> + *      NOT(XOR(a,b))=XOR(XOR(a,b),1)

Yes, well, XNOR = tcg_gen_eqv_tl, which is what tcg_gen_vec_sub16_i64 uses.
Unfortunately, we only have 64-bit implementations of these generically.

> +        default:
> +            arc_debug_opcode(opcode, ctx, "No handle for map opcode");
> +            g_assert(!"Semantic not handled: Use -d unimp to list it.");

This must raise an exception, or return false, or something.


r~