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

[Richard Henderson <rth@twiddle.net>]

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?

> +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_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.


r~