From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45940) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THJSy-0004NU-Lw for qemu-devel@nongnu.org; Thu, 27 Sep 2012 15:09:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1THJSs-0006vT-Rq for qemu-devel@nongnu.org; Thu, 27 Sep 2012 15:09:44 -0400 Received: from mail-da0-f45.google.com ([209.85.210.45]:37115) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THJSs-0006vK-L4 for qemu-devel@nongnu.org; Thu, 27 Sep 2012 15:09:38 -0400 Received: by dadn15 with SMTP id n15so458826dad.4 for ; Thu, 27 Sep 2012 12:09:37 -0700 (PDT) Sender: Richard Henderson Message-ID: <5064A46F.30105@twiddle.net> Date: Thu, 27 Sep 2012 12:09:35 -0700 From: Richard Henderson MIME-Version: 1.0 References: <1348766113-18373-1-git-send-email-aurelien@aurel32.net> <1348766113-18373-8-git-send-email-aurelien@aurel32.net> In-Reply-To: <1348766113-18373-8-git-send-email-aurelien@aurel32.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/13] tcg: rewrite tcg_reg_alloc_mov() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aurelien Jarno Cc: qemu-devel@nongnu.org On 09/27/2012 10:15 AM, Aurelien Jarno wrote: > + /* We have to load the value in a register for moving it to another > + or for saving it. We assume it's better to keep it there than to > + reload it later. */ > + if ((NEED_SYNC_ARG(0) && ts->val_type != TEMP_VAL_REG) > + || ts->val_type == TEMP_VAL_MEM) { > + ts->reg = tcg_reg_alloc(s, arg_ct->u.regs, allocated_regs); > + if (ts->val_type == TEMP_VAL_MEM) { > + tcg_out_ld(s, ts->type, ts->reg, ts->mem_reg, ts->mem_offset); > + } else if (ts->val_type == TEMP_VAL_CONST) { > + tcg_out_movi(s, ts->type, ts->reg, ts->val); > + } > + s->reg_to_temp[ts->reg] = args[1]; > + ts->val_type = TEMP_VAL_REG; > + ts->mem_coherent = 1; > + } I don't understand this block. In particular, the ts->mem_coherent = 1 in the TEMP_VAL_CONST case looks wrong. Why are you handling NEED_SYNC_ARG before the move, rather than after? > + if (IS_DEAD_ARG(0) && !ots->fixed_reg) { > + /* mov to a non-saved dead register makes no sense (even with > + liveness analysis disabled). */ > + assert(NEED_SYNC_ARG(0)); > + /* The code above should have moved the temp to a register. */ > + assert(ts->val_type == TEMP_VAL_REG); > + if (!ots->mem_allocated) { > + temp_allocate_frame(s, args[0]); > } > + if (ts->val_type == TEMP_VAL_REG) { > + tcg_out_st(s, ots->type, ts->reg, ots->mem_reg, ots->mem_offset); > + if (IS_DEAD_ARG(1)) { > + temp_dead(s, args[1]); > + } > } > + temp_dead(s, args[0]); Isn't this store going to happen via temp_dead -> temp_sync -> tcg_reg_sync? > } else if (ts->val_type == TEMP_VAL_CONST) { > if (ots->fixed_reg) { > + tcg_out_movi(s, ots->type, ots->reg, ts->val); > + s->reg_to_temp[ots->reg] = args[0]; > } else { > /* propagate constant */ > + if (ots->val_type == TEMP_VAL_REG) { > s->reg_to_temp[ots->reg] = -1; > + } > ots->val_type = TEMP_VAL_CONST; > ots->val = ts->val; > } How much of the first block above is redundant with this? Especially given that I think there's a missing sync. > } else { > + /* The code above should have moved the temp to a register. */ > + assert(ts->val_type == TEMP_VAL_REG); > + if (IS_DEAD_ARG(1) && !ts->fixed_reg && !ots->fixed_reg) { > + /* the mov can be suppressed */ > + if (ots->val_type == TEMP_VAL_REG) { > + s->reg_to_temp[ots->reg] = -1; > + } > + ots->reg = ts->reg; > + temp_dead(s, args[1]); > + } else { > + if (ots->val_type != TEMP_VAL_REG) { > + /* When allocating a new register, make sure to not spill the > + input one. */ > + tcg_regset_set_reg(allocated_regs, ts->reg); > + ots->reg = tcg_reg_alloc(s, oarg_ct->u.regs, allocated_regs); > + } > + tcg_out_mov(s, ots->type, ots->reg, ts->reg); > + } > + ots->val_type = TEMP_VAL_REG; > + ots->mem_coherent = 0; > + s->reg_to_temp[ots->reg] = args[0]; > + if (NEED_SYNC_ARG(0)) { > + tcg_reg_sync(s, ots->reg); > + } ... as we do here. r~