Re: [Qemu-devel] [PATCH v2 11/14] tcg: Implement indirect memory registers

[Aurelien Jarno <aurelien@aurel32.net>]

On 2015-12-17 12:00, Richard Henderson wrote:
> That is, global_mem registers whose base is another global_mem
> register, rather than a fixed register.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/tcg.c | 95 ++++++++++++++++++++++++++++++++++++++++++++-------------------
>  tcg/tcg.h |  2 ++
>  2 files changed, 68 insertions(+), 29 deletions(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index c51e0ec..7150a3f 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -509,17 +509,23 @@ int tcg_global_mem_new_internal(TCGType type, TCGv_ptr base,
>      TCGContext *s = &tcg_ctx;
>      TCGTemp *base_ts = &s->temps[GET_TCGV_PTR(base)];
>      TCGTemp *ts = tcg_global_alloc(s);
> -    int bigendian = 0;
> +    int indirect_reg = 0, bigendian = 0;
>  #ifdef HOST_WORDS_BIGENDIAN
>      bigendian = 1;
>  #endif
>  
> +    if (!base_ts->fixed_reg) {
> +        indirect_reg = 1;
> +        base_ts->indirect_base = 1;
> +    }
> +
>      if (TCG_TARGET_REG_BITS == 32 && type == TCG_TYPE_I64) {
>          TCGTemp *ts2 = tcg_global_alloc(s);
>          char buf[64];
>  
>          ts->base_type = TCG_TYPE_I64;
>          ts->type = TCG_TYPE_I32;
> +        ts->indirect_reg = indirect_reg;

Do we really need to add this new bit, while we can simply test
ts->mem_base->fixed_reg? This means one more derefence, but anyway it
has to be done later when calling temp_load?

>          ts->mem_allocated = 1;
>          ts->mem_base = base_ts;
>          ts->mem_offset = offset + bigendian * 4;

> @@ -1781,13 +1803,16 @@ static inline void temp_save(TCGContext *s, TCGTemp *ts,
>                               TCGRegSet allocated_regs)
>  {
>  #ifdef USE_LIVENESS_ANALYSIS
> -    /* The liveness analysis already ensures that globals are back
> -       in memory. Keep an assert for safety. */
> -    tcg_debug_assert(ts->val_type == TEMP_VAL_MEM || ts->fixed_reg);
> -#else
> +    /* ??? Liveness does not yet incorporate indirect bases.  */
> +    if (!ts->indirect_base) {
> +        /* The liveness analysis already ensures that globals are back
> +           in memory. Keep an assert for safety. */
> +        tcg_debug_assert(ts->val_type == TEMP_VAL_MEM || ts->fixed_reg);
> +        return;
> +    }

This basically disables the assert. What does it mean in practice? Can
it generates bad code?

> +#endif
>      temp_sync(s, ts, allocated_regs);
>      temp_dead(s, ts);
> -#endif
>  }
>  
>  /* save globals to their canonical location and assume they can be
> @@ -1812,12 +1837,15 @@ static void sync_globals(TCGContext *s, TCGRegSet allocated_regs)
>      for (i = 0; i < s->nb_globals; i++) {
>          TCGTemp *ts = &s->temps[i];
>  #ifdef USE_LIVENESS_ANALYSIS
> -        tcg_debug_assert(ts->val_type != TEMP_VAL_REG
> -                         || ts->fixed_reg
> -                         || ts->mem_coherent);
> -#else
> -        temp_sync(s, ts, allocated_regs);
> +        /* ??? Liveness does not yet incorporate indirect bases.  */
> +        if (!ts->indirect_base) {
> +            tcg_debug_assert(ts->val_type != TEMP_VAL_REG
> +                             || ts->fixed_reg
> +                             || ts->mem_coherent);
> +            continue;
> +        }

Same here.

>  #endif
> +        temp_sync(s, ts, allocated_regs);
>      }
>  }
>  
> @@ -1833,12 +1861,15 @@ static void tcg_reg_alloc_bb_end(TCGContext *s, TCGRegSet allocated_regs)
>              temp_save(s, ts, allocated_regs);
>          } else {
>  #ifdef USE_LIVENESS_ANALYSIS
> -            /* The liveness analysis already ensures that temps are dead.
> -               Keep an assert for safety. */
> -            assert(ts->val_type == TEMP_VAL_DEAD);
> -#else
> -            temp_dead(s, ts);
> +            /* ??? Liveness does not yet incorporate indirect bases.  */
> +            if (!ts->indirect_base) {
> +                /* The liveness analysis already ensures that temps are dead.
> +                   Keep an assert for safety. */
> +                assert(ts->val_type == TEMP_VAL_DEAD);
> +                continue;
> +            }

And here.

>  #endif
> +            temp_dead(s, ts);
>          }
>      }
>  

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