Re: [Qemu-devel] [PATCH v3 7/9] tcg: Compress dead_temps and mem_temps into a single array

[Aurelien Jarno <aurelien@aurel32.net>]

On 2016-06-23 20:48, Richard Henderson wrote:
> We only need two bits per temporary.  Fold the two bytes into one,
> and reduce the memory and cachelines required during compilation.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/tcg.c | 118 +++++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 59 insertions(+), 59 deletions(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index c41640f..fd92b06 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -332,7 +332,7 @@ void tcg_context_init(TCGContext *s)
>  
>      memset(s, 0, sizeof(*s));
>      s->nb_globals = 0;
> -    
> +
>      /* Count total number of arguments and allocate the corresponding
>         space */
>      total_args = 0;
> @@ -824,16 +824,16 @@ void tcg_gen_callN(TCGContext *s, void *func, TCGArg ret,
>                  real_args++;
>              }
>  #endif
> -	    /* If stack grows up, then we will be placing successive
> -	       arguments at lower addresses, which means we need to
> -	       reverse the order compared to how we would normally
> -	       treat either big or little-endian.  For those arguments
> -	       that will wind up in registers, this still works for
> -	       HPPA (the only current STACK_GROWSUP target) since the
> -	       argument registers are *also* allocated in decreasing
> -	       order.  If another such target is added, this logic may
> -	       have to get more complicated to differentiate between
> -	       stack arguments and register arguments.  */
> +           /* If stack grows up, then we will be placing successive
> +              arguments at lower addresses, which means we need to
> +              reverse the order compared to how we would normally
> +              treat either big or little-endian.  For those arguments
> +              that will wind up in registers, this still works for
> +              HPPA (the only current STACK_GROWSUP target) since the
> +              argument registers are *also* allocated in decreasing
> +              order.  If another such target is added, this logic may
> +              have to get more complicated to differentiate between
> +              stack arguments and register arguments.  */
>  #if defined(HOST_WORDS_BIGENDIAN) != defined(TCG_TARGET_STACK_GROWSUP)
>              s->gen_opparam_buf[pi++] = args[i] + 1;
>              s->gen_opparam_buf[pi++] = args[i];
> @@ -1297,27 +1297,28 @@ void tcg_op_remove(TCGContext *s, TCGOp *op)
>  #endif
>  }
>  
> +#define TS_DEAD  1
> +#define TS_SYNC  2

I am not sure that TS_SYNC is the best name for that. There we really
want to tell that at this moment in the TCG instruction stream the
operand is in memory. It doesn't implied it is synced. Maybe TS_MEM?

The rest looks fine to me. The other alternative would have been to use
the TCGTempSet with the bitmap functions like in optimize.c, and use
only 2 bits per temp. That something that can be done later though.

All that said and provided you change the name:

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

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