| Message ID | 1348766113-18373-8-git-send-email-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
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~
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
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; > + } Ok, I believe I understand what's going on now. Nothing like trying to rewrite the function yourself to figure out why you've done things this way. The only thing I'd change for clarity is that first condition: /* If the source value is not in a register, and we're going to be forced to have it in a register in order to perform the copy, then copy the SOURCE value into its own register first. That way we don't have to reload SOURCE the next time it is used. Note that in the CONST + SYNC case, we must for the moment have the value in a register because we have no direct access to a store constant primitive. */ if ((ts->val_type == TEMP_VAL_CONST && NEED_SYNC_ARG(0)) || ts->val_type == TEMP_VAL_MEM) { r~
diff --git a/tcg/tcg.c b/tcg/tcg.c index bfe6411..5fb4901 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -1667,64 +1667,85 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def, const TCGArg *args, uint16_t dead_args, uint8_t sync_args) { + TCGRegSet allocated_regs; TCGTemp *ts, *ots; - int reg; - const TCGArgConstraint *arg_ct; + const TCGArgConstraint *arg_ct, *oarg_ct; + tcg_regset_set(allocated_regs, s->reserved_regs); ots = &s->temps[args[0]]; ts = &s->temps[args[1]]; - arg_ct = &def->args_ct[0]; + oarg_ct = &def->args_ct[0]; + arg_ct = &def->args_ct[1]; + + /* 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; + } - /* XXX: always mark arg dead if IS_DEAD_ARG(1) */ - if (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; - reg = ts->reg; - temp_dead(s, args[1]); - } else { - if (ots->val_type == TEMP_VAL_REG) { - reg = ots->reg; - } else { - reg = tcg_reg_alloc(s, arg_ct->u.regs, s->reserved_regs); - } - if (ts->reg != reg) { - tcg_out_mov(s, ots->type, reg, ts->reg); - } + 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]); } - } else if (ts->val_type == TEMP_VAL_MEM) { - if (ots->val_type == TEMP_VAL_REG) { - reg = ots->reg; - } else { - reg = tcg_reg_alloc(s, arg_ct->u.regs, s->reserved_regs); + 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]); + } } - tcg_out_ld(s, ts->type, reg, ts->mem_reg, ts->mem_offset); + temp_dead(s, args[0]); } else if (ts->val_type == TEMP_VAL_CONST) { if (ots->fixed_reg) { - reg = ots->reg; - tcg_out_movi(s, ots->type, reg, ts->val); + 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) + if (ots->val_type == TEMP_VAL_REG) { s->reg_to_temp[ots->reg] = -1; + } ots->val_type = TEMP_VAL_CONST; ots->val = ts->val; - if (NEED_SYNC_ARG(0)) { - temp_sync(s, args[0], s->reserved_regs); - } - return; } } else { - tcg_abort(); - } - s->reg_to_temp[reg] = args[0]; - ots->reg = reg; - ots->val_type = TEMP_VAL_REG; - ots->mem_coherent = 0; - - if (NEED_SYNC_ARG(0)) { - tcg_reg_sync(s, reg); + /* 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); + } } }
Now that the liveness analysis provides more information, rewrite tcg_reg_alloc_mov(). This changes the behaviour about propagating constants and memory accesses. We now take the assumption that once a value is loaded into a register (from memory or from a constant), it's better to keep it there than to reload it later. This assumption is now always almost correct given that we are now sure the corresponding temp is going to be used later (otherwise it would have been synchronized and marked as dead already). The assumption is wrong if one of the op after clobbers some registers including the one of the holding the temp (this can be avoided by allocating clobbered registers last, which is what most TCG target do), or in case of lack of available register. Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- tcg/tcg.c | 105 ++++++++++++++++++++++++++++++++++++------------------------- 1 file changed, 63 insertions(+), 42 deletions(-)