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~
