Re: [PATCH v2 09/36] tcg: Consolidate 3 bits into enum TCGTempKind

["Alex Bennée" <alex.bennee@linaro.org>]

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

> The temp_fixed, temp_global, temp_local bits are all related.
> Combine them into a single enumeration.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/tcg/tcg.h |  20 +++++---
>  tcg/optimize.c    |   8 +--
>  tcg/tcg.c         | 122 ++++++++++++++++++++++++++++------------------
>  3 files changed, 90 insertions(+), 60 deletions(-)
>
> diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
> index c48bd76b0a..3534dce77f 100644
> --- a/include/tcg/tcg.h
> +++ b/include/tcg/tcg.h
> @@ -480,23 +480,27 @@ typedef enum TCGTempVal {
>      TEMP_VAL_CONST,
>  } TCGTempVal;
>  
> +typedef enum TCGTempKind {
> +    /* Temp is dead at the end of all basic blocks. */
> +    TEMP_NORMAL,
> +    /* Temp is saved across basic blocks but dead at the end of TBs. */
> +    TEMP_LOCAL,
> +    /* Temp is saved across both basic blocks and translation blocks. */
> +    TEMP_GLOBAL,
> +    /* Temp is in a fixed register. */
> +    TEMP_FIXED,
> +} TCGTempKind;
> +
<snip>
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -116,21 +116,21 @@ static TCGTemp *find_better_copy(TCGContext *s, TCGTemp *ts)
>      TCGTemp *i;
>  
>      /* If this is already a global, we can't do better. */
> -    if (ts->temp_global) {
> +    if (ts->kind >= TEMP_GLOBAL) {
>          return ts;
>      }
>  
>      /* Search for a global first. */
>      for (i = ts_info(ts)->next_copy; i != ts; i = ts_info(i)->next_copy) {
> -        if (i->temp_global) {
> +        if (i->kind >= TEMP_GLOBAL) {
>              return i;
>          }
>      }
>  
>      /* If it is a temp, search for a temp local. */
> -    if (!ts->temp_local) {
> +    if (ts->kind == TEMP_NORMAL) {
>          for (i = ts_info(ts)->next_copy; i != ts; i = ts_info(i)->next_copy) {
> -            if (ts->temp_local) {
> +            if (i->kind >= TEMP_LOCAL) {
>                  return i;
>              }

I was confused as to why these were not equality tests as being of one
type does not imply the properties of another? But I see the logic is
simplified even more in later patches.

<snip>
>  
>      memset(s->reg_to_temp, 0, sizeof(s->reg_to_temp));
> @@ -1885,12 +1896,17 @@ static char *tcg_get_arg_str_ptr(TCGContext *s, char *buf, int buf_size,
>  {
>      int idx = temp_idx(ts);
>  
> -    if (ts->temp_global) {
> +    switch (ts->kind) {
> +    case TEMP_FIXED:
> +    case TEMP_GLOBAL:
>          pstrcpy(buf, buf_size, ts->name);
> -    } else if (ts->temp_local) {
> +        break;
> +    case TEMP_LOCAL:
>          snprintf(buf, buf_size, "loc%d", idx - s->nb_globals);
> -    } else {
> +        break;
> +    case TEMP_NORMAL:
>          snprintf(buf, buf_size, "tmp%d", idx - s->nb_globals);
> +        break;
>      }
>      return buf;

Random aside - if tcg is firmly staying part of qemu we should consider
modernising some of the string handling here.

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée