Re: [Qemu-devel] [PATCH 2/8] tcg: Use a GHashTable for tcg_find_helper

[Stefan Weil]

Am 15.09.2013 02:03, schrieb Richard Henderson:
> Slightly changes the interface, in that we now return name
> instead of a TCGHelperInfo structure, which goes away.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  tcg/tcg.c | 74 
> ++++++++++++++++-----------------------------------------------
>  tcg/tcg.h | 10 +--------
>  2 files changed, 19 insertions(+), 65 deletions(-)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index fd7fb6b..98b1c37 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -623,20 +623,15 @@ int tcg_check_temp_count(void)
>  void tcg_register_helper(void *func, const char *name)
>  {
>      TCGContext *s = &tcg_ctx;
> -    int n;
> -    if ((s->nb_helpers + 1) > s->allocated_helpers) {
> -        n = s->allocated_helpers;
> -        if (n == 0) {
> -            n = 4;
> -        } else {
> -            n *= 2;
> -        }
> -        s->helpers = realloc(s->helpers, n * sizeof(TCGHelperInfo));
> -        s->allocated_helpers = n;
> +    GHashTable *table = s->helpers;
> +
> +    if (table == NULL) {
> +        /* Use g_direct_hash/equal for direct pointer comparisons on func.  
> */
> +        table = g_hash_table_new(NULL, NULL);
> +        s->helpers = table;
>      }
> -    s->helpers[s->nb_helpers].func = (uintptr_t)func;
> -    s->helpers[s->nb_helpers].name = name;
> -    s->nb_helpers++;
> +
> +    g_hash_table_insert(table, (gpointer)func, (gpointer)name);
>  }
>  
>  /* Note: we convert the 64 bit args to 32 bit and do some alignment
> @@ -851,47 +846,14 @@ char *tcg_get_arg_str_i64(TCGContext *s, char *buf, int 
> buf_size, TCGv_i64 arg)
>      return tcg_get_arg_str_idx(s, buf, buf_size, GET_TCGV_I64(arg));
>  }
>  
> -static int helper_cmp(const void *p1, const void *p2)
> +/* Find helper name.  */
> +static inline const char *tcg_find_helper(TCGContext *s, uintptr_t val)
>  {
> -    const TCGHelperInfo *th1 = p1;
> -    const TCGHelperInfo *th2 = p2;
> -    if (th1->func < th2->func)
> -        return -1;
> -    else if (th1->func == th2->func)
> -        return 0;
> -    else
> -        return 1;
> -}
> -
> -/* find helper definition (Note: A hash table would be better) */
> -static TCGHelperInfo *tcg_find_helper(TCGContext *s, uintptr_t val)
> -{
> -    int m, m_min, m_max;
> -    TCGHelperInfo *th;
> -    uintptr_t v;
> -
> -    if (unlikely(!s->helpers_sorted)) {
> -        qsort(s->helpers, s->nb_helpers, sizeof(TCGHelperInfo), 
> -              helper_cmp);
> -        s->helpers_sorted = 1;
> -    }
> -
> -    /* binary search */
> -    m_min = 0;
> -    m_max = s->nb_helpers - 1;
> -    while (m_min <= m_max) {
> -        m = (m_min + m_max) >> 1;
> -        th = &s->helpers[m];
> -        v = th->func;
> -        if (v == val)
> -            return th;
> -        else if (val < v) {
> -            m_max = m - 1;
> -        } else {
> -            m_min = m + 1;
> -        }
> +    const char *ret = NULL;
> +    if (s->helpers) {
> +     ret = g_hash_table_lookup(s->helpers, (gpointer)val);

Please replace tab by spaces here.

>      }
> -    return NULL;
> +    return ret;
>  }
>  
>  static const char * const cond_name[] =
> @@ -976,7 +938,7 @@ void tcg_dump_ops(TCGContext *s)
>              }
>          } else if (c == INDEX_op_movi_i32 || c == INDEX_op_movi_i64) {
>              tcg_target_ulong val;
> -            TCGHelperInfo *th;
> +            const char *name;
>  
>              nb_oargs = def->nb_oargs;
>              nb_iargs = def->nb_iargs;
> @@ -984,9 +946,9 @@ void tcg_dump_ops(TCGContext *s)
>              qemu_log(" %s %s,$", def->name,
>                       tcg_get_arg_str_idx(s, buf, sizeof(buf), args[0]));
>              val = args[1];
> -            th = tcg_find_helper(s, val);
> -            if (th) {
> -                qemu_log("%s", th->name);
> +            name = tcg_find_helper(s, val);
> +            if (name) {
> +                qemu_log("%s", name);
>              } else {
>                  if (c == INDEX_op_movi_i32) {
>                      qemu_log("0x%x", (uint32_t)val);
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 20543f6..8c5eb42 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -405,11 +405,6 @@ typedef struct TCGTemp {
>      const char *name;
>  } TCGTemp;
>  
> -typedef struct TCGHelperInfo {
> -    uintptr_t func;
> -    const char *name;
> -} TCGHelperInfo;
> -
>  typedef struct TCGContext TCGContext;
>  
>  struct TCGContext {
> @@ -447,10 +442,7 @@ struct TCGContext {
>      uint8_t *code_ptr;
>      TCGTemp temps[TCG_MAX_TEMPS]; /* globals first, temps after */
>  
> -    TCGHelperInfo *helpers;
> -    int nb_helpers;
> -    int allocated_helpers;
> -    int helpers_sorted;
> +    GHashTable *helpers;
>  
>  #ifdef CONFIG_PROFILER
>      /* profiling info */

With fixed tab (see above):

Reviewed-by: Stefan Weil <address@hidden>