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

[WANG Xuerui <i.qemu@xen0n.name>]

Hi Philippe,

On 9/25/21 17:54, Philippe Mathieu-Daudé wrote:
> On 9/24/21 19:25, WANG Xuerui wrote:
>> Signed-off-by: WANG Xuerui <git@xen0n.name>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   tcg/loongarch64/tcg-target.c.inc | 109 +++++++++++++++++++++++++++++++
>>   1 file changed, 109 insertions(+)
>
>> +/* Loads a 32-bit immediate into rd, sign-extended.  */
>> +static void tcg_out_movi_i32(TCGContext *s, TCGReg rd, int32_t val)
>> +{
>> +    /* Single-instruction cases.  */
>> +    tcg_target_long lo = sextreg(val, 0, 12);
>> +    if (lo == val) {
>> +        /* val fits in simm12: addi.w rd, zero, val */
>> +        tcg_out_opc_addi_w(s, rd, TCG_REG_ZERO, val);
>> +        return;
>> +    }
>> +    if (0x800 <= val && val <= 0xfff) {
>> +        /* val fits in uimm12: ori rd, zero, val */
>> +        tcg_out_opc_ori(s, rd, TCG_REG_ZERO, val);
>> +        return;
>> +    }
>> +
>> +    /* 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.
Sure; will fix in v6.
>
> Maybe name lo12 and hi20?
I added ASCII art to hopefully clarify the namings; originally I used 
MIPS R6 terms (low, upper, higher, top) but all the MIPS R6 instructions 
take 16-bit imm, so I figured just naming by bitfield LSB index would be 
best. The Loongson documentation people didn't invent any dedicated name 
for the 4 parts or 3 load-upper instructions, either.
>
>> +    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?
As Richard explained, lo is signed while ori takes unsigned imm, so this 
is necessary to not trip up the debug assert and overwrite the opcode.
>
>> +    }
>> +}
>> +
>> +static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd,
>> +                         tcg_target_long val)
>> +{
>> +    if (type == TCG_TYPE_I32 || val == (int32_t)val) {
>> +        tcg_out_movi_i32(s, rd, val);
>> +        return;
>> +    }
>> +
>> +    /* PC-relative cases.  */
>> +    intptr_t pc_offset = tcg_pcrel_diff(s, (void *)val);
>
> Declare in prologue.
Ack; will fix.
>
>> +    if (pc_offset == sextreg(pc_offset, 0, 22) && (pc_offset & 3) == 
>> 0) {
>> +        /* Single pcaddu2i.  */
>> +        tcg_out_opc_pcaddu2i(s, rd, pc_offset >> 2);
>> +        return;
>> +    }
>> +
>> +    if (pc_offset == (int32_t)pc_offset) {
>> +        /* Offset within 32 bits; load with pcalau12i + ori. */
>> +        tcg_target_long lo = sextreg(val, 0, 12);
>
> Name this 'val_lo' similarly to val_hi?
Nice catch; that was leftover from v4 where the tcg_out_movi_i32 logic 
was not factored out. Fixed in v6.
>
>> +        tcg_target_long pc_hi = (val - pc_offset) >> 12;
>> +        tcg_target_long val_hi = val >> 12;
>> +        tcg_target_long offset_hi = val_hi - pc_hi;
>> +        tcg_debug_assert(offset_hi == sextreg(offset_hi, 0, 20));
>> +        tcg_out_opc_pcalau12i(s, rd, offset_hi);
>> +        if (lo != 0) {
>> +            tcg_out_opc_ori(s, rd, rd, lo & 0xfff);
>
> Again, lo is already 12-bit.
Same as above.
>
>> +        }
>> +        return;
>> +    }
>> +
>> +    /* Single cu52i.d case.  */
>> +    if (ctz64(val) >= 52) {
>> +        tcg_out_opc_cu52i_d(s, rd, TCG_REG_ZERO, val >> 52);
>> +        return;
>> +    }
>> +
>> +    /* 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:
>
>        rd_high_bits_are_ones = (int32_t)val < 0 ? true : false;
Hmm, comparison operators return boolean results already; and I thought 
expressions like `foo ? true : false` are typically considered to have 
"bad smell"? I think I'd want to keep the current way of saying 
things... But I'll of course move the declaration to function prologue.
>
>> +    tcg_target_long hi32 = sextreg(val, 32, 20);
>
> By 'hi32' I expect the 32 high bits. Maybe explicit as hi32_20?
>
>> +    tcg_target_long hi52 = sextreg(val, 52, 12);
>
> And hi52_12?
The names are getting long with addition of bitfield lengths; I hope the 
ASCII art in v6 could alleviate the need for longer names.
>
>> +
>> +    if (imm_part_needs_loading(rd_high_bits_are_ones, hi32)) {
>> +        tcg_out_opc_cu32i_d(s, rd, hi32);
>> +        rd_high_bits_are_ones = hi32 < 0;
>
> Again KISS:
>
>            if (hi32 < 0) {
>                rd_high_bits_are_ones = true;
>            }
>
As explained by Richard, this is indeed meant to be an unconditional 
overwrite. After concatenating the 51-to-32 bits, the topmost 12 bits is 
changed to be sign-extension of hi32, so the flag must be updated to 
reflect that.
>> +    }
>> +
>> +    if (imm_part_needs_loading(rd_high_bits_are_ones, hi52)) {
>> +        tcg_out_opc_cu52i_d(s, rd, rd, hi52);
>> +    }
>> +}
>
> With comment addressed:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>