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

[Richard Henderson <rth@twiddle.net>]

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.

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

> +    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?

>      } 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.

>      } 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.


r~