[24/26] tcg: Introduce tcg_temp_ebb_new_*

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(-)

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.

On 11/30/22 10:07, Ilya Leoshkevich wrote:
> 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.

Thanks for the review on this.

I'm not sure I want to take this anymore.  It's confusing to use.  I really think what I 
should do instead is improve the TCG register allocator.


r~

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

> On 11/30/22 10:07, Ilya Leoshkevich wrote:
>> 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.
>
> Thanks for the review on this.
>
> I'm not sure I want to take this anymore.  It's confusing to use.  I
> really think what I should do instead is improve the TCG register
> allocator.

Whats the ultimate aim for the rewrite? Hold values in target registers
over the extended block? What about avoid spills between potential fault
points?

>
>
> r~

On 12/1/22 11:13, Alex Bennée wrote:
>> I'm not sure I want to take this anymore.  It's confusing to use.  I
>> really think what I should do instead is improve the TCG register
>> allocator.
> 
> Whats the ultimate aim for the rewrite? Hold values in target registers
> over the extended block? What about avoid spills between potential fault
> points?

Primary goal is to handle register allocation across basic blocks without front end 
translators needing to consider "local" vs "temporary".  Which would also allow any use of 
branches in the middle-end, where we currently have to worry about accidentally killing 
temporaries held by the front end.  That's where this patch fails, in that it makes the 
current situation even more complicated.

Secondary goal would be proper register allocation across basic blocks.

Tertiary goal would be single-assignment form, to allow better optimizations.

I'll have to do some research before committing to do anything in this area.


r~