Re: [Qemu-devel] [PATCH v3 2/3] tcg/aarch64: implement new TCG target for aarch64

[Claudio Fontana]

On 28.05.2013 18:18, Richard Henderson wrote:
> On 05/28/2013 08:28 AM, Claudio Fontana wrote:
>> +static inline void tcg_out_movi_aux(TCGContext *s,
>> +                                    TCGReg rd, uint64_t value)
>> +{
>> +    uint32_t half, base, movk = 0, shift = 0;
>> +
>> +    /* construct halfwords of the immediate with MOVZ/MOVK with LSL */
>> +    /* using MOVZ 0x52800000 | extended reg.. */
>> +    base = (value > 0xffffffff) ? 0xd2800000 : 0x52800000;
>> +
>> +    do {
>> +        int skip_zeros = ctz64(value) & (63 & -16);
>> +        value >>= skip_zeros;
>> +        shift += skip_zeros << 17;
>> +        half = value & 0xffff;
>> +        tcg_out32(s, base | movk | shift | half << 5 | rd);
>> +        movk = 0x20000000; /* morph next MOVZs into MOVKs */
>> +        value >>= 16;
>> +        shift += 16 << 17;
> 
> This is way more confusing than it needs to be.  I don't think you
> should modify VALUE by shifting at all.  If you don't do that then
> you don't need to make SHIFT loop carried, since we compute its
> exact correct value every time with the ctz.
> 
> Was the only bug in the code that I pasted the lack of the shift-by-17
> when encoding SHIFT into the tcg_out32?

yes, you only forgot to encode the shift in the tcg_out32,
the variation above was an attempt to make it easier to understand.
I agree that the approach that avoids changing value in the right shift
is more concise, I'll go back to that, adding a comment about how the
function works.

>> +static inline void tcg_out_movi(TCGContext *s, TCGType type,
>> +                                TCGReg rd, tcg_target_long value)
>> +{
>> +    if (type == TCG_TYPE_I64) {
>> +        tcg_out_movi_aux(s, rd, value);
>> +    } else {
>> +        tcg_out_movi_aux(s, rd, value & 0xffffffff);
>> +    }
>> +}
> 
> Any reason you're splitting out tcg_out_movi_aux to a separate function?

tcg_out_movi is an interface with tcg, and as such the prototype is fixed.
I'd rather work with a value that is unsigned, because of the right shift.
Having a separate _aux function does that without the need for adding
another local variable and another operation to understand in the function
with the actual algorithm.

> 
>> +    tcg_regset_clear(s->reserved_regs);
>> +    tcg_regset_set_reg(s->reserved_regs, TCG_REG_SP);
>> +    tcg_regset_set_reg(s->reserved_regs, TCG_REG_TMP);
>> +    tcg_regset_set_reg(s->reserved_regs, TCG_REG_X18); /* platform register 
>> */
> 
> Reserve the frame pointer.

Ok.

> r~

Claudio