From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60593) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QNULT-0004o8-J5 for qemu-devel@nongnu.org; Fri, 20 May 2011 14:22:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QNULP-0000yf-Cj for qemu-devel@nongnu.org; Fri, 20 May 2011 14:22:43 -0400 Received: from mail-pz0-f45.google.com ([209.85.210.45]:49590) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QNULP-0000yZ-6P for qemu-devel@nongnu.org; Fri, 20 May 2011 14:22:39 -0400 Received: by pzk30 with SMTP id 30so2223312pzk.4 for ; Fri, 20 May 2011 11:22:38 -0700 (PDT) Sender: Richard Henderson Message-ID: <4DD6B16C.5090500@twiddle.net> Date: Fri, 20 May 2011 11:22:36 -0700 From: Richard Henderson MIME-Version: 1.0 References: <4763ae5461ae14adbb6aca5c925fa0fe81f4f214.1305889001.git.batuzovk@ispras.ru> In-Reply-To: <4763ae5461ae14adbb6aca5c925fa0fe81f4f214.1305889001.git.batuzovk@ispras.ru> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/6] Add copy and constant propagation. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirill Batuzov Cc: mj.mccormack@samsung.com, qemu-devel@nongnu.org, zhur@ispras.ru 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~