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

[Richard Henderson <richard.henderson@linaro.org>]

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~