Re: [Qemu-devel] [PATCH 07/13] tcg: rewrite tcg_reg_alloc_mov()

[Aurelien Jarno]

On Thu, Sep 27, 2012 at 12:09:35PM -0700, Richard Henderson wrote:
> On 09/27/2012 10:15 AM, Aurelien Jarno wrote:
> > +    /* We have to load the value in a register for moving it to another
> > +       or for saving it. We assume it's better to keep it there than to
> > +       reload it later. */
> > +    if ((NEED_SYNC_ARG(0) && ts->val_type != TEMP_VAL_REG)
> > +        || ts->val_type == TEMP_VAL_MEM) {
> > +        ts->reg = tcg_reg_alloc(s, arg_ct->u.regs, allocated_regs);
> > +        if (ts->val_type == TEMP_VAL_MEM) {
> > +            tcg_out_ld(s, ts->type, ts->reg, ts->mem_reg, ts->mem_offset);
> > +        } else if (ts->val_type == TEMP_VAL_CONST) {
> > +            tcg_out_movi(s, ts->type, ts->reg, ts->val);
> > +        }
> > +        s->reg_to_temp[ts->reg] = args[1];
> > +        ts->val_type = TEMP_VAL_REG;
> > +        ts->mem_coherent = 1;
> > +    }
> 
> I don't understand this block.  In particular, the ts->mem_coherent = 1
> in the TEMP_VAL_CONST case looks wrong.

Indeed the ts->mem_coherent = 1 is wrong in the TEMP_VAL_CONST.

> Why are you handling NEED_SYNC_ARG before the move, rather than after?

Because the move is likely to be eliminated by the code below,
especially if the temp is dead in addition.

> > +    if (IS_DEAD_ARG(0) && !ots->fixed_reg) {
> > +        /* mov to a non-saved dead register makes no sense (even with
> > +           liveness analysis disabled). */
> > +        assert(NEED_SYNC_ARG(0));
> > +        /* The code above should have moved the temp to a register. */
> > +        assert(ts->val_type == TEMP_VAL_REG);
> > +        if (!ots->mem_allocated) {
> > +            temp_allocate_frame(s, args[0]);
> >          }
> > +        if (ts->val_type == TEMP_VAL_REG) {
> > +            tcg_out_st(s, ots->type, ts->reg, ots->mem_reg, 
> > ots->mem_offset);
> > +            if (IS_DEAD_ARG(1)) {
> > +                temp_dead(s, args[1]);
> > +            }
> >          }
> > +        temp_dead(s, args[0]);
> 
> Isn't this store going to happen via temp_dead -> temp_sync -> tcg_reg_sync?

temp_dead only mark the temp as dead, it doesn't save it.

> >      } else if (ts->val_type == TEMP_VAL_CONST) {
> >          if (ots->fixed_reg) {
> > +            tcg_out_movi(s, ots->type, ots->reg, ts->val);
> > +            s->reg_to_temp[ots->reg] = args[0];
> >          } else {
> >              /* propagate constant */
> > +            if (ots->val_type == TEMP_VAL_REG) {
> >                  s->reg_to_temp[ots->reg] = -1;
> > +            }
> >              ots->val_type = TEMP_VAL_CONST;
> >              ots->val = ts->val;
> >          }
> 
> How much of the first block above is redundant with this?
> Especially given that I think there's a missing sync.

The goal of to first block is to move the value to a register in case a
sync is needed, and the sync is done at this moment. The ots->fixed_reg
can indeed by merged into the first block, but the rest has to stay there.

> >      } else {
> > +        /* The code above should have moved the temp to a register. */
> > +        assert(ts->val_type == TEMP_VAL_REG);
> > +        if (IS_DEAD_ARG(1) && !ts->fixed_reg && !ots->fixed_reg) {
> > +                /* the mov can be suppressed */
> > +                if (ots->val_type == TEMP_VAL_REG) {
> > +                    s->reg_to_temp[ots->reg] = -1;
> > +                }
> > +                ots->reg = ts->reg;
> > +                temp_dead(s, args[1]);
> > +        } else {
> > +            if (ots->val_type != TEMP_VAL_REG) {
> > +                /* When allocating a new register, make sure to not spill 
> > the
> > +                   input one. */
> > +                tcg_regset_set_reg(allocated_regs, ts->reg);
> > +                ots->reg = tcg_reg_alloc(s, oarg_ct->u.regs, 
> > allocated_regs);
> > +            }
> > +            tcg_out_mov(s, ots->type, ots->reg, ts->reg);
> > +        }
> > +        ots->val_type = TEMP_VAL_REG;
> > +        ots->mem_coherent = 0;
> > +        s->reg_to_temp[ots->reg] = args[0];
> > +        if (NEED_SYNC_ARG(0)) {
> > +            tcg_reg_sync(s, ots->reg);
> > +        }
> 
> ... as we do here.

The sync is done here because it has not been done in the first block.
It's different than in the ts->val_type == TEMP_VAL_CONST


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net