Re: [PATCH v5 09/30] tcg/loongarch64: Implement tcg_out_mov and tcg_out_movi

[Richard Henderson]

On 9/25/21 5:54 AM, Philippe Mathieu-Daudé wrote:
> > +    /* High bits must be set; load with lu12i.w + optional ori.  */
> > +    tcg_target_long hi12 = sextreg(val, 12, 20);
>
> Please declare variables in function prologue.

Ah, true.  Officially, that's qemu coding style.  I tend to overlook it because I don't 
like the rule.


> > +    tcg_out_opc_lu12i_w(s, rd, hi12);
> > +    if (lo != 0) {
> > +        tcg_out_opc_ori(s, rd, rd, lo & 0xfff);
>
> Isn't lo already 12-bit? Why the mask?

lo was extracted signed, for addi; ori wants the value unsigned.
The value range asserts in tcg_out_opc_* will notice the difference.

> > +    /* Slow path.  Initialize the low 32 bits, then concat high bits.  */
> > +    tcg_out_movi_i32(s, rd, val);
> > +
> > +    bool rd_high_bits_are_ones = (int32_t)val < 0;
>
> Declare in prologue, however this is hard to read. KISS:

If anything was to change here, I think I'd simply re-use "lo":

    lo = (int32_t)val;

and then check lo < 0 rather than rd_high_bits_are_ones as a boolean.

> > +        rd_high_bits_are_ones = hi32 < 0;
>
> Again KISS:
>
>             if (hi32 < 0) {
>                 rd_high_bits_are_ones = true;
>             }

This is certainly not KISS, and also wrong.  We would want an unconditional write to 
rd_high_bits_are_ones.  Although if there were any change,

    lo = hi32.

so that, again, we test lo < 0 for hi52.


r~