Re: [Qemu-devel] [PATCH v2 09/29] tcg: Manually expand INDEX_op_dup_vec

[Richard Henderson]

On 5/2/19 2:42 AM, Alex Bennée wrote:
>> +static void tcg_reg_alloc_dup(TCGContext *s, const TCGOp *op)
>> +{
>> +    const TCGLifeData arg_life = op->life;
>> +    TCGRegSet dup_out_regs, dup_in_regs;
>> +    TCGTemp *its, *ots;
>> +    TCGType itype, vtype;
>> +    unsigned vece;
>> +    bool ok;
>> +
>> +    ots = arg_temp(op->args[0]);
>> +    its = arg_temp(op->args[1]);
>> +
>> +    /* There should be no fixed vector registers.  */
>> +    tcg_debug_assert(!ots->fixed_reg);
> 
> This threw me slightly. I guess you only really duplicate vectors so I'm
> wondering if this should be called tcg_vec_reg_alloc_dup? Or maybe just
> a bit of verbiage in a block comment above the helper?

Perhaps just a bit more verbiage.

The convention so far is "tcg_reg_alloc_<opcode>", where so far mov, movi, and
call have specialized allocators.  Everything else happens in tcg_reg_alloc_op.
 So tcg_reg_alloc_dup is correct for handling dup.


>> +    case TEMP_VAL_MEM:
>> +        /* TODO: dup from memory */
>> +        tcg_out_ld(s, itype, ots->reg, its->mem_base->reg,
>> its->mem_offset);
> 
> Should we be aborting here? That said it looks like you are loading
> something directly from the register memory address here...

No, we should not abort.  We load the scalar value into a register that we have
allocated that matches the input constraint for dup.  We then fall through...

> 
>> +        break;
>> +
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    /* We now have a vector input register, so dup must succeed. */
>> +    ok = tcg_out_dup_vec(s, vtype, vece, ots->reg, ots->reg);
>> +    tcg_debug_assert(ok);

... to here, where we duplicate the scalar across the vector.
Success.

The TODO comment is about duplicating directly from the memory slot, with a new
dupm primitive, which appears in the next patch.


r~