Re: [Qemu-devel] [PATCH 2/6] Add copy and constant propagation.

[Richard Henderson]

On 05/20/2011 05:39 AM, Kirill Batuzov wrote:
> +    static tcg_target_ulong vals[TCG_MAX_TEMPS];
> +    static tcg_temp_state state[TCG_MAX_TEMPS];

Any particular reason to make these static?  It's 4-6k on the host
stack depending on sizeof tcg_target_ulong.  Large, but not excessive.
I think it's just confusing to have them static, myself.

I think it might be clearer to have these two in a structure, rather
than two separate arrays.  That does waste a bit of memory in padding
for 64-bit target, but I think it might clean up the code a bit.

>      nb_temps = s->nb_temps;
>      nb_globals = s->nb_globals;
> +    memset(state, 0, nb_temps * sizeof(tcg_temp_state));

Of course, instead of allocating static structures, you could alloca
the memory in the appropriate size...

> +        case INDEX_op_mov_i32:
> +#if TCG_TARGET_REG_BITS == 64
> +        case INDEX_op_mov_i64:
> +#endif
> +            if ((state[args[1]] == TCG_TEMP_COPY
> +                && vals[args[1]] == args[0])
> +                || args[0] == args[1]) {
> +                args += 2;
> +                gen_opc_buf[op_index] = INDEX_op_nop;
> +                break;

Here, for example, INDEX_op_nop2 would be more appropriate,
obviating the need for the arg copy loop from patch 1.

> +            if (state[args[1]] != TCG_TEMP_CONST) {
> +            } else {
> +                /* Source argument is constant.  Rewrite the operation and
> +                   let movi case handle it. */
> +            }

FWIW, I think writing positive tests is clearer than writing
negative tests.  I.e. reverse the condition and the if/else bodies.

>          case INDEX_op_brcond_i64:
>  #endif
> +            memset(state, 0, nb_temps * sizeof(tcg_temp_state));

Why are you resetting at the branch, rather than at the label?
Seems reasonable enough to handle the extended basic block when
possible...


r~