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
[email protected] http://www.aurel32.net
