Re: [Qemu-devel] [PATCH RFC v20 5/8] target/avr: Add instruction translation

[Richard Henderson <richard.henderson@linaro.org>]

On 6/1/19 10:44 PM, Michael Rolnik wrote:
> Hi Richard.
> 
> these instructions are not branches or jumps they all do skip.

Of course they're not all branches.  I used the example of a branch to show a
situation in which your translation is wrong.

> however, if you think it's important I change it, I will, just show me an
> example or explain.

Ok, let's take HPPA as a very similar example.

Many HPPA instructions may "nullify" the next instruction.  The language is
different, but it's the same thing as the AVR "skip".

Now, I spent quite a bit of effort in target/hppa using conditional move
instructions to implement this.  But you need not go that far.

Now, AVR is differs from HPPA in that there is a nullify bit as part of the
process state.  Since AVR does not have this, we will need to keep the "skip"
state entirely internal to the qemu implementation.

I suggest:

(1) Add "bool skipping;" to CPUAVRState.

Because TranslationBlocks can be ended at any instruction boundary, we need
some way to preserve the skipping state across TB's.

(2) Include "skipping" into the flags for cpu_get_tb_cpu_state, TB_FLAGS_SKIPPING.

(3) Include "skipping" into the computation of cpu_interrupts_enabled.

Because "skipping" is not part of the architectural state of the CPU, we cannot
allow an interrupt to come between the two instructions.  Therefore, while
"skipping" is true, disable interrupts.

(4) Within the instructions that skip the next, issue the branch but record the
label as DisasContext->skip_label.  This will allow the main loop (and other
instructions) know that skipping is active.

(5a) In gen_intermediate_code, if TB_FLAGS_SKIPPING, decode but do not
translate the insn, then clear TB_FLAGS_SKIPPING and store 0 into env->skipping.

(5b) In gen_intermediate_code, if !TB_FLAGS_SKIPPING, copy
DisasContext->skip_label into a local variable, this_skip_label and zero.

We need to prepare for skip of skip, so do not allow the label of the first
skip to be clobbered by the label of the second skip.

(5c) After translate(), if this_skip_label is non-null, emit the label.

(6) Reverse the sense of your conditional branches.

Currently you generate

  brcond(xxx, yyy, zzz, true_label);
  goto npc
true_label:
  goto true_pc

which is fine until we have skip labels.  We now want to emit

  brcond(!xxx, yyy, zzz, false_label);
  goto true_pc
false_label:
skip_label:
  goto npc

which you can do by issuing only the branch, goto, label, and then setting
ctx->bstate to BS_STOP, so that the skip_label is emitted by the main loop, and
the goto npc is also issued by the main loop.

(7) At the end of the loop in gen_intermedite_code, if DisasContext->skip_label
is non-null, then we ended the TB with a skipping instruction and we need to
preserve that within env.

    TCGLabel *finish = NULL;

    if (ctx.skip_label) {
        finish = gen_new_label();
    }

    if (tb->cflags & CF_LAST_IO) {
    ...


    if (ctx.skip_label) {
        TCGv_i32 one;

        gen_set_label(ctx.skip_label);
        one = tcg_const_i32(1);
        tcg_gen_st8_i32(one, cpu_env, offsetof(CPUAVRState, skipping));
        tcg_temp_free_i32(one);
        tcg_gen_br(finish);
    }

 done_generating:
   gen_tb_end(tb, num_insns);


r~