On 11/11/20 10:17 AM, cupertinomira...@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~
