Re: [PATCH v2 11/36] tcg: Introduce TYPE_CONST temporaries

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> These will hold a single constant for the duration of the TB.
> They are hashed, so that each value has one temp across the TB.
>
> Not used yet, this is all infrastructure.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/tcg/tcg.h |  27 ++++++++++-
>  tcg/optimize.c    |  40 ++++++++++-------
>  tcg/tcg-op-vec.c  |  17 +++++++
>  tcg/tcg.c         | 111 +++++++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 166 insertions(+), 29 deletions(-)
>
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index 27e1b509a6..f72530dfda 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -489,6 +489,8 @@ typedef enum TCGTempKind {
>      TEMP_GLOBAL,
>      /* Temp is in a fixed register. */
>      TEMP_FIXED,
> +    /* Temp is a fixed constant. */
> +    TEMP_CONST,
>  } TCGTempKind;
>  
>  typedef struct TCGTemp {
> @@ -664,6 +666,7 @@ struct TCGContext {
>      QSIMPLEQ_HEAD(, TCGOp) plugin_ops;
>  #endif
>  
> +    GHashTable *const_table[TCG_TYPE_COUNT];
>      TCGTempSet free_temps[TCG_TYPE_COUNT * 2];
>      TCGTemp temps[TCG_MAX_TEMPS]; /* globals first, temps after */
>  
> @@ -680,7 +683,7 @@ struct TCGContext {
>  
>  static inline bool temp_readonly(TCGTemp *ts)
>  {
> -    return ts->kind == TEMP_FIXED;
> +    return ts->kind >= TEMP_FIXED;

I should have clarified in the previous patch - TEMP_FIXED is a fixed
value, e.g. env or other internal pointer which we won't be overwriting
or otherwise trashing anywhere in the block?

>  }
>  
>  extern TCGContext tcg_init_ctx;
> @@ -1038,6 +1041,7 @@ TCGOp *tcg_op_insert_after(TCGContext *s, TCGOp *op, TCGOpcode opc);
>  
>  void tcg_optimize(TCGContext *s);
>  
> +/* Allocate a new temporary and initialize it with a constant. */
>  TCGv_i32 tcg_const_i32(int32_t val);
>  TCGv_i64 tcg_const_i64(int64_t val);
>  TCGv_i32 tcg_const_local_i32(int32_t val);
> @@ -1047,6 +1051,27 @@ TCGv_vec tcg_const_ones_vec(TCGType);
>  TCGv_vec tcg_const_zeros_vec_matching(TCGv_vec);
>  TCGv_vec tcg_const_ones_vec_matching(TCGv_vec);
>  
> +/*
> + * Locate or create a read-only temporary that is a constant.
> + * This kind of temporary need not and should not be freed.
> + */
> +TCGTemp *tcg_constant_internal(TCGType type, tcg_target_long val);
> +
> +static inline TCGv_i32 tcg_constant_i32(int32_t val)
> +{
> +    return temp_tcgv_i32(tcg_constant_internal(TCG_TYPE_I32, val));
> +}
> +
> +static inline TCGv_i64 tcg_constant_i64(int64_t val)
> +{
> +    if (TCG_TARGET_REG_BITS == 32) {
> +        qemu_build_not_reached();
> +    }
> +    return temp_tcgv_i64(tcg_constant_internal(TCG_TYPE_I64, val));
> +}
> +
> +TCGv_vec tcg_constant_vec(TCGType type, unsigned vece, int64_t val);
> +
>  #if UINTPTR_MAX == UINT32_MAX
>  # define tcg_const_ptr(x)        ((TCGv_ptr)tcg_const_i32((intptr_t)(x)))
>  # define tcg_const_local_ptr(x)  ((TCGv_ptr)tcg_const_local_i32((intptr_t)(x)))
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index afb4a9a5a9..effb47eefd 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -99,8 +99,17 @@ static void init_ts_info(struct tcg_temp_info *infos,
>          ts->state_ptr = ti;
>          ti->next_copy = ts;
>          ti->prev_copy = ts;
> -        ti->is_const = false;
> -        ti->mask = -1;
> +        if (ts->kind == TEMP_CONST) {
> +            ti->is_const = true;
> +            ti->val = ti->mask = ts->val;
> +            if (TCG_TARGET_REG_BITS > 32 && ts->type == TCG_TYPE_I32) {
> +                /* High bits of a 32-bit quantity are garbage.  */
> +                ti->mask |= ~0xffffffffull;
> +            }
> +        } else {
> +            ti->is_const = false;
> +            ti->mask = -1;
> +        }
>          set_bit(idx, temps_used->l);
>      }
>  }
> @@ -113,31 +122,28 @@ static void init_arg_info(struct tcg_temp_info *infos,
>  
>  static TCGTemp *find_better_copy(TCGContext *s, TCGTemp *ts)
>  {
> -    TCGTemp *i;
> +    TCGTemp *i, *g, *l;
>  
> -    /* If this is already a global, we can't do better. */
> -    if (ts->kind >= TEMP_GLOBAL) {
> +    /* If this is already readonly, we can't do better. */
> +    if (temp_readonly(ts)) {
>          return ts;
>      }
>  
> -    /* Search for a global first. */
> +    g = l = NULL;
>      for (i = ts_info(ts)->next_copy; i != ts; i = ts_info(i)->next_copy) {
> -        if (i->kind >= TEMP_GLOBAL) {
> +        if (temp_readonly(i)) {
>              return i;
> -        }
> -    }
> -
> -    /* If it is a temp, search for a temp local. */
> -    if (ts->kind == TEMP_NORMAL) {
> -        for (i = ts_info(ts)->next_copy; i != ts; i = ts_info(i)->next_copy) {
> -            if (i->kind >= TEMP_LOCAL) {
> -                return i;
> +        } else if (i->kind > ts->kind) {
> +            if (i->kind == TEMP_GLOBAL) {
> +                g = i;
> +            } else if (i->kind == TEMP_LOCAL) {
> +                l = i;
>              }
>          }
>      }
>  
> -    /* Failure to find a better representation, return the same temp. */
> -    return ts;
> +    /* If we didn't find a better representation, return the same temp. */
> +    return g ? g : l ? l : ts;
>  }
>  
>  static bool ts_are_copies(TCGTemp *ts1, TCGTemp *ts2)
> diff --git a/tcg/tcg-op-vec.c b/tcg/tcg-op-vec.c
> index b6937e8d64..f3927089a7 100644
> --- a/tcg/tcg-op-vec.c
> +++ b/tcg/tcg-op-vec.c
> @@ -209,6 +209,23 @@ static void vec_gen_op3(TCGOpcode opc, unsigned vece,
>      vec_gen_3(opc, type, vece, temp_arg(rt), temp_arg(at), temp_arg(bt));
>  }
>  
> +TCGv_vec tcg_constant_vec(TCGType type, unsigned vece, int64_t val)
> +{
> +    val = dup_const(vece, val);
> +
> +    /*
> +     * For MO_64 constants that can't be represented in tcg_target_long,
> +     * we must use INDEX_op_dup2_vec, which requires a non-const temporary.
> +     */
> +    if (TCG_TARGET_REG_BITS == 32 &&
> +        val != deposit64(val, 32, 32, val) &&
> +        val != (uint64_t)(int32_t)val) {
> +        g_assert_not_reached();
> +    }
> +
> +    return temp_tcgv_vec(tcg_constant_internal(type, val));
> +}
> +
>  void tcg_gen_mov_vec(TCGv_vec r, TCGv_vec a)
>  {
>      if (r != a) {
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 92b3767097..59beb2bf29 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1127,6 +1127,7 @@ void tcg_func_start(TCGContext *s)
>  
>      /* No temps have been previously allocated for size or locality.  */
>      memset(s->free_temps, 0, sizeof(s->free_temps));
> +    memset(s->const_table, 0, sizeof(s->const_table));
>  
>      s->nb_ops = 0;
>      s->nb_labels = 0;
> @@ -1199,13 +1200,19 @@ TCGTemp *tcg_global_mem_new_internal(TCGType type, TCGv_ptr base,
>      bigendian = 1;
>  #endif
>  
> -    if (base_ts->kind != TEMP_FIXED) {
> +    switch (base_ts->kind) {
> +    case TEMP_FIXED:
> +        break;
> +    case TEMP_GLOBAL:
>          /* We do not support double-indirect registers.  */
>          tcg_debug_assert(!base_ts->indirect_reg);
>          base_ts->indirect_base = 1;
>          s->nb_indirects += (TCG_TARGET_REG_BITS == 32 && type == TCG_TYPE_I64
>                              ? 2 : 1);
>          indirect_reg = 1;
> +        break;
> +    default:
> +        g_assert_not_reached();
>      }
>  
>      if (TCG_TARGET_REG_BITS == 32 && type == TCG_TYPE_I64) {
> @@ -1346,6 +1353,37 @@ void tcg_temp_free_internal(TCGTemp *ts)
>      set_bit(idx, s->free_temps[k].l);
>  }
>  
> +TCGTemp *tcg_constant_internal(TCGType type, tcg_target_long val)
> +{
> +    TCGContext *s = tcg_ctx;
> +    GHashTable *h = s->const_table[type];
> +    TCGTemp *ts;
> +
> +    if (h == NULL) {
> +        if (sizeof(tcg_target_long) == sizeof(gint64)) {
> +            h = g_hash_table_new(g_int64_hash, g_int64_equal);
> +        } else if (sizeof(tcg_target_long) == sizeof(gint)) {
> +            h = g_hash_table_new(g_int_hash, g_int_equal);
> +        } else {
> +            qemu_build_not_reached();
> +        }
> +        s->const_table[type] = h;
> +    }
> +
> +    ts = g_hash_table_lookup(h, &val);
> +    if (ts == NULL) {
> +        ts = tcg_temp_alloc(s);
> +        ts->base_type = type;
> +        ts->type = type;
> +        ts->kind = TEMP_CONST;
> +        ts->temp_allocated = 1;
> +        ts->val = val;
> +        g_hash_table_insert(h, &ts->val, ts);

I worried about the efficiency of using a hash table for a low number of
constants but glib's implementation starts with 8 buckets and then
scales up so it seems a reasonable approach.

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée