From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38433) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THKWd-0005zK-NV for qemu-devel@nongnu.org; Thu, 27 Sep 2012 16:17:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1THKWc-00057Z-6g for qemu-devel@nongnu.org; Thu, 27 Sep 2012 16:17:35 -0400 Received: from hall.aurel32.net ([88.191.126.93]:36455) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THKWc-00057D-0d for qemu-devel@nongnu.org; Thu, 27 Sep 2012 16:17:34 -0400 Date: Thu, 27 Sep 2012 22:17:28 +0200 From: Aurelien Jarno Message-ID: <20120927201728.GK20151@ohm.aurel32.net> References: <1348766113-18373-1-git-send-email-aurelien@aurel32.net> <1348766113-18373-8-git-send-email-aurelien@aurel32.net> <5064A46F.30105@twiddle.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <5064A46F.30105@twiddle.net> 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: Richard Henderson Cc: qemu-devel@nongnu.org On Thu, Sep 27, 2012 at 12:09:35PM -0700, Richard Henderson wrote: > 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. Indeed the ts->mem_coherent = 1 is wrong in the TEMP_VAL_CONST. > Why are you handling NEED_SYNC_ARG before the move, rather than after? Because the move is likely to be eliminated by the code below, especially if the temp is dead in addition. > > + 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? temp_dead only mark the temp as dead, it doesn't save it. > > } 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. The goal of to first block is to move the value to a register in case a sync is needed, and the sync is done at this moment. The ots->fixed_reg can indeed by merged into the first block, but the rest has to stay there. > > } 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. The sync is done here because it has not been done in the first block. It's different than in the ts->val_type == TEMP_VAL_CONST -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net