Re: [Qemu-devel] [PATCH 11/13] target-openrisc: use jmp_pc as flag variable for branches

[Sebastian Macke]

On 30/10/2013 11:33 AM, Richard Henderson wrote:
> On 10/29/2013 12:04 PM, Sebastian Macke wrote:
> >           {
> >               int lab = gen_new_label();
> > -            dc->btaken = tcg_temp_local_new();
> > -            tcg_gen_movi_tl(jmp_pc, dc->pc+8);
> > -            tcg_gen_movi_tl(dc->btaken, 0);
> > +            tcg_gen_movi_tl(jmp_pc, 0);
> >               tcg_gen_brcondi_i32(op0 == 0x03 ? TCG_COND_NE : TCG_COND_EQ,
> >                                   cpu_srf, 0, lab);
> > -            tcg_gen_movi_tl(dc->btaken, 1);
> >               tcg_gen_movi_tl(jmp_pc, tmp_pc);
> >               gen_set_label(lab);
> You can now use movcond here:
>
>    tcg_gen_movi_i32(jmp_pc, tmp_pc);
>    tcg_gen_movcond_i32(jmp_pc, cpu_srf, zero, jmp_pc, zero,
>                        op0 == 0x03 ? TCG_COND_NE : TCG_COND_EQ);
>
> Although I'd wonder about just using setcond instead, since I think
> the value stored in jmp_pc here is also stored in dc->j_target, leading
> to the right behaviour...

Correct. The movcond function can be used here. I will change that.
But I didn't know that it exists because it is not written in the 
document http://wiki.qemu.org/Documentation/TCG/frontend-ops


> >       case JUMP_BRANCH:
> >           {
> >               int l1 = gen_new_label();
> > -            tcg_gen_brcondi_tl(TCG_COND_NE, dc->btaken, 0, l1);
> > +            tcg_gen_brcondi_tl(TCG_COND_NE, jmp_pc, 0, l1);
> >               gen_goto_tb(dc, 1, dc->pc);
> >               gen_set_label(l1);
> > -            tcg_temp_free(dc->btaken);
> >               gen_goto_tb(dc, 0, dc->j_target);
> >               break;
> ... here.

But j_target is not known when the delayed slot is translated 
separately. (E.g. if the delayed slot is at a page boundary.)

> > +    case JUMP_BRANCH_DELAYED:
> > +        {
> > +            int l1 = gen_new_label();
> > +            tcg_gen_brcondi_tl(TCG_COND_NE, jmp_pc, 0, l1);
> > +            gen_goto_tb(dc, 1, dc->pc);
> > +            gen_set_label(l1);
> > +            tcg_gen_mov_tl(cpu_pc, jmp_pc);
> > +            tcg_gen_exit_tb(0);
> > +            break;
> ...
> > +
> > +    dc->delayed_branch = !!(dc->tb_flags & D_FLAG);
> > +    if ((dc->delayed_branch) && (dc->tb_flags&B_FLAG)) {
> > +        dc->j_state = JUMP_BRANCH_DELAYED;
> > +    }
> > +
>
> And thus I can't see how these additions are actually useful?
>
> If I've missed something, and the last hunk needs to be retained,
> then please fix the coding style:
>
>    if (dc->delayed_branch && (dc->tb_flags & B_FLAG)) {
The problem is the delayed slot.
The decision whether a jump is taken or not depends on the previous 
branch instruction l.bf and l.bnf instruction.
If only the delayed slot is translated then we have to know what was the 
previous instruction.
In this case we have to differ additionally if the delayed slot is a 
branch or a normal jump. Because for a branch the jmp_pc field is also a 
flag.

We could simply jump to jmp_pc like it was done before but then we miss 
the block chaining feature.
And I have not seen another way implementing it.




> r~