Re: [PATCH 06/15] arc: TCG instruction definitions

[Cupertino Miranda]

>> +
>> +    assert(ctx->insn.limm_p == 0 && !in_delay_slot);
>> +
>> +    if (ctx->insn.limm_p == 0 && !in_delay_slot) {
>> +        in_delay_slot = true;
>> +        uint32_t cpc = ctx->cpc;
>> +        uint32_t pcl = ctx->pcl;
>> +        insn_t insn = ctx->insn;
>> +
>> +        ctx->cpc = ctx->npc;
>> +        ctx->pcl = ctx->cpc & 0xfffffffc;
>> +
>> +        ++ctx->ds;
>> +
>> +        TCGLabel *do_not_set_bta_and_de = gen_new_label();
>> +        tcg_gen_brcondi_i32(TCG_COND_NE, take_branch, 1, 
>> do_not_set_bta_and_de);
>> +        /*
>> +         * In case an exception should be raised during the execution
>> +         * of delay slot, bta value is used to set erbta.
>> +         */
>> +        tcg_gen_mov_tl(cpu_bta, bta);
>> +        /* We are in a delay slot */
>> +        tcg_gen_mov_tl(cpu_DEf, take_branch);
>> +        gen_set_label(do_not_set_bta_and_de);
>> +
>> +        tcg_gen_movi_tl(cpu_is_delay_slot_instruction, 1);
>> +
>> +        /* Set the pc to the next pc */
>> +        tcg_gen_movi_tl(cpu_pc, ctx->npc);
>> +        /* Necessary for the likely call to restore_state_to_opc() */
>> +        tcg_gen_insn_start(ctx->npc);
> 
> Wow, this looks wrong.
> 
> It doesn't work with icount or single-stepping.  You need to be able to begin 
> a
> TB at any instruction, including a delay slot.
> 
> You should have a look at some of the other targets that handle this, e.g.
> openrisc or sparc.  Since you've got NPC already, for looping, sparc is
> probably the better match.
> 
> There should be no reason why you'd need to emit insn_start outside of
> arc_tr_insn_start.
> 

We are also able to start TB at any instruction, that is not what is 
being validated here. It is just verifying if a delayslot instruction 
does not have the delayslot flag set, and that the delayslot instruction 
does not have a limm.

The way we encode delayslot instructions is a little peculiar. When some 
instruction defines a delayslot, it calls this function and the 
instruction is decoded inline.
As far as I remember the change of instruction context, with 
tcg_gen_insns_start, allowed us to properly make gdb jump from branch 
instruction to delay slot instruction and then back.

The asser was used to guarantee that those conditions where never broken 
internally. The condition becomes irrelevant.


>> + ***************************************
>> + * Statically inferred return function *
>> + ***************************************
>> + */
>> +
>> +TCGv arc_gen_next_reg(const DisasCtxt *ctx, TCGv reg)
>> +{
>> +    int i;
>> +    for (i = 0; i < 64; i += 2) {
>> +        if (reg == cpu_r[i]) {
>> +            return cpu_r[i + 1];
>> +        }
>> +    }
>> +    /* Check if REG is an odd register. */
>> +    for (i = 1; i < 64; i += 2) {
>> +        /* If so, that is unsanctioned. */
>> +        if (reg == cpu_r[i]) {
>> +            arc_gen_excp(ctx, EXCP_INST_ERROR, 0, 0);
>> +            return NULL;
>> +        }
>> +    }
> 
> Wow, really?  Can't you grab this directly from the operand decoding?  Where
> surely we have already ensured that the relevant regnos are not odd.

Indeed, we can grab it from operand decoding. Please allow us to do this 
change once we rework binutils code.