Re: [PATCH 24/26] tcg: Introduce tcg_temp_ebb_new_*

From: Ilya Leoshkevich <iii@linux.ibm.com>
To: Richard Henderson <richard.henderson@linaro.org>, qemu-devel@nongnu.org
Cc: qemu-s390x@nongnu.org
Subject: Re: [PATCH 24/26] tcg: Introduce tcg_temp_ebb_new_*
Date: Wed, 30 Nov 2022 19:07:03 +0100	[thread overview]
Message-ID: <20221130180703.dprt7lzmppgfihtp@heavy> (raw)
In-Reply-To: <20221006034421.1179141-25-richard.henderson@linaro.org>

On Wed, Oct 05, 2022 at 08:44:19PM -0700, Richard Henderson wrote:
> Allow targets to allocate extended-basic-block temps.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/tcg/tcg-op.h |  2 ++
>  include/tcg/tcg.h    | 20 +++++++++++++++++++-
>  tcg/tcg.c            | 16 ++++------------
>  3 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/include/tcg/tcg-op.h b/include/tcg/tcg-op.h
> index 209e168305..0ebbee6e24 100644
> --- a/include/tcg/tcg-op.h
> +++ b/include/tcg/tcg-op.h
> @@ -848,6 +848,7 @@ static inline void tcg_gen_plugin_cb_end(void)
>  #define tcg_temp_new() tcg_temp_new_i32()
>  #define tcg_global_mem_new tcg_global_mem_new_i32
>  #define tcg_temp_local_new() tcg_temp_local_new_i32()
> +#define tcg_temp_ebb_new() tcg_temp_ebb_new_i32()
>  #define tcg_temp_free tcg_temp_free_i32
>  #define tcg_gen_qemu_ld_tl tcg_gen_qemu_ld_i32
>  #define tcg_gen_qemu_st_tl tcg_gen_qemu_st_i32
> @@ -855,6 +856,7 @@ static inline void tcg_gen_plugin_cb_end(void)
>  #define tcg_temp_new() tcg_temp_new_i64()
>  #define tcg_global_mem_new tcg_global_mem_new_i64
>  #define tcg_temp_local_new() tcg_temp_local_new_i64()
> +#define tcg_temp_ebb_new() tcg_temp_ebb_new_i64()
>  #define tcg_temp_free tcg_temp_free_i64
>  #define tcg_gen_qemu_ld_tl tcg_gen_qemu_ld_i64
>  #define tcg_gen_qemu_st_tl tcg_gen_qemu_st_i64
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index e01a47ec20..3835711d52 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -609,7 +609,7 @@ struct TCGContext {
>  #endif
>  
>      GHashTable *const_table[TCG_TYPE_COUNT];
> -    TCGTempSet free_temps[TCG_TYPE_COUNT * 2];
> +    TCGTempSet free_temps[TCG_TYPE_COUNT * 3];
>      TCGTemp temps[TCG_MAX_TEMPS]; /* globals first, temps after */
>  
>      QTAILQ_HEAD(, TCGOp) ops, free_ops;
> @@ -890,6 +890,12 @@ static inline TCGv_i32 tcg_temp_local_new_i32(void)
>      return temp_tcgv_i32(t);
>  }
>  
> +static inline TCGv_i32 tcg_temp_ebb_new_i32(void)
> +{
> +    TCGTemp *t = tcg_temp_new_internal(TCG_TYPE_I32, TEMP_EBB);
> +    return temp_tcgv_i32(t);
> +}
> +
>  static inline TCGv_i64 tcg_global_mem_new_i64(TCGv_ptr reg, intptr_t offset,
>                                                const char *name)
>  {
> @@ -909,6 +915,12 @@ static inline TCGv_i64 tcg_temp_local_new_i64(void)
>      return temp_tcgv_i64(t);
>  }
>  
> +static inline TCGv_i64 tcg_temp_ebb_new_i64(void)
> +{
> +    TCGTemp *t = tcg_temp_new_internal(TCG_TYPE_I64, TEMP_EBB);
> +    return temp_tcgv_i64(t);
> +}
> +
>  static inline TCGv_ptr tcg_global_mem_new_ptr(TCGv_ptr reg, intptr_t offset,
>                                                const char *name)
>  {
> @@ -928,6 +940,12 @@ static inline TCGv_ptr tcg_temp_local_new_ptr(void)
>      return temp_tcgv_ptr(t);
>  }
>  
> +static inline TCGv_ptr tcg_temp_ebb_new_ptr(void)
> +{
> +    TCGTemp *t = tcg_temp_new_internal(TCG_TYPE_PTR, TEMP_EBB);
> +    return temp_tcgv_ptr(t);
> +}
> +
>  #if defined(CONFIG_DEBUG_TCG)
>  /* If you call tcg_clear_temp_count() at the start of a section of
>   * code which is not supposed to leak any TCG temporaries, then
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index acdbd5a9a2..7aa6cc3451 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -948,17 +948,8 @@ TCGTemp *tcg_temp_new_internal(TCGType type, TCGTempKind kind)
>      TCGTemp *ts;
>      int idx, k;
>  
> -    switch (kind) {
> -    case TEMP_NORMAL:
> -        k = 0;
> -        break;
> -    case TEMP_LOCAL:
> -        k = TCG_TYPE_COUNT;
> -        break;
> -    default:
> -        g_assert_not_reached();
> -    }
> -    k += type;
> +    assert(kind >= TEMP_NORMAL && kind <= TEMP_LOCAL);

Nit: maybe also add QEMU_BUILD_BUG_ON(TEMP_NORMAL != 0)
and QEMU_BUILD_BUG_ON(TEMP_LOCAL != 2), since we are using this for
0-based array indexing here? Alternatively, subtract TEMP_NORMAL
from kind.

> +    k = TCG_TYPE_COUNT * kind + type;
>  
>      idx = find_first_bit(s->free_temps[k].l, TCG_MAX_TEMPS);
>      if (idx < TCG_MAX_TEMPS) {
> @@ -1046,6 +1037,7 @@ void tcg_temp_free_internal(TCGTemp *ts)
>           */
>          return;
>      case TEMP_NORMAL:
> +    case TEMP_EBB:
>      case TEMP_LOCAL:
>          break;
>      default:
> @@ -1063,7 +1055,7 @@ void tcg_temp_free_internal(TCGTemp *ts)
>      ts->temp_allocated = 0;
>  
>      idx = temp_idx(ts);
> -    k = ts->base_type + (ts->kind == TEMP_NORMAL ? 0 : TCG_TYPE_COUNT);
> +    k = ts->base_type + ts->kind * TCG_TYPE_COUNT;
>      set_bit(idx, s->free_temps[k].l);
>  }
>  
> -- 
> 2.34.1
> 
> 

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>

While not directly related to this patch, it would be good to update
tcg/README with all the new kinds of temporaries. E.g. the EBB ones are
not mentioned there:

    TCG instructions operate on variables which are temporaries, local
    temporaries or globals.