Re: [Qemu-devel] [PATCH v2 10/14] tcg: Introduce temp_load

[Aurelien Jarno <aurelien@aurel32.net>]

On 2015-12-17 12:00, Richard Henderson wrote:
> Unify all of the places that realize a temporary into a register.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/tcg.c | 116 ++++++++++++++++++++++++++++----------------------------------
>  1 file changed, 52 insertions(+), 64 deletions(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 6f40bf3..c51e0ec 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c

[ snip ]

> @@ -2162,23 +2163,9 @@ static void tcg_reg_alloc_call(TCGContext *s, int nb_oargs, int nb_iargs,
>  #endif
>          if (arg != TCG_CALL_DUMMY_ARG) {
>              ts = &s->temps[arg];
> -            if (ts->val_type == TEMP_VAL_REG) {
> -                tcg_out_st(s, ts->type, ts->reg, TCG_REG_CALL_STACK, stack_offset);
> -            } else if (ts->val_type == TEMP_VAL_MEM) {
> -                reg = tcg_reg_alloc(s, tcg_target_available_regs[ts->type], 
> -                                    s->reserved_regs);
> -                /* XXX: not correct if reading values from the stack */

Shouldn't we keep this comment by moving it to temp_load?

> -                tcg_out_ld(s, ts->type, reg, ts->mem_base->reg, ts->mem_offset);
> -                tcg_out_st(s, ts->type, reg, TCG_REG_CALL_STACK, stack_offset);
> -            } else if (ts->val_type == TEMP_VAL_CONST) {
> -                reg = tcg_reg_alloc(s, tcg_target_available_regs[ts->type], 
> -                                    s->reserved_regs);
> -                /* XXX: sign extend may be needed on some targets */

Same here.

> -                tcg_out_movi(s, ts->type, reg, ts->val);
> -                tcg_out_st(s, ts->type, reg, TCG_REG_CALL_STACK, stack_offset);
> -            } else {
> -                tcg_abort();
> -            }
> +            temp_load(s, ts, tcg_target_available_regs[ts->type],
> +                      s->reserved_regs);
> +            tcg_out_st(s, ts->type, ts->reg, TCG_REG_CALL_STACK, stack_offset);
>          }
>  #ifndef TCG_TARGET_STACK_GROWSUP
>          stack_offset += sizeof(tcg_target_long);
> @@ -2193,18 +2180,19 @@ static void tcg_reg_alloc_call(TCGContext *s, int nb_oargs, int nb_iargs,
>              ts = &s->temps[arg];
>              reg = tcg_target_call_iarg_regs[i];
>              tcg_reg_free(s, reg);
> +
>              if (ts->val_type == TEMP_VAL_REG) {
>                  if (ts->reg != reg) {
>                      tcg_out_mov(s, ts->type, reg, ts->reg);
>                  }
> -            } else if (ts->val_type == TEMP_VAL_MEM) {
> -                tcg_out_ld(s, ts->type, reg, ts->mem_base->reg, ts->mem_offset);
> -            } else if (ts->val_type == TEMP_VAL_CONST) {
> -                /* XXX: sign extend ? */

And here.

> -                tcg_out_movi(s, ts->type, reg, ts->val);
>              } else {
> -                tcg_abort();
> +                TCGRegSet arg_set;
> +
> +                tcg_regset_clear(arg_set);
> +                tcg_regset_set_reg(arg_set, reg);
> +                temp_load(s, ts, arg_set, allocated_regs);
>              }
> +
>              tcg_regset_set_reg(allocated_regs, reg);
>          }
>      }

Otherwise it's a nice cleanup :-).

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net