Re: [PATCH v5 09/30] tcg/loongarch64: Implement tcg_out_mov and tcg_out_movi - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: WANG Xuerui <git@xen0n.name>, qemu-devel@nongnu.org
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Richard Henderson <richard.henderson@linaro.org>,
	Laurent Vivier <laurent@vivier.eu>
Subject: Re: [PATCH v5 09/30] tcg/loongarch64: Implement tcg_out_mov and tcg_out_movi
Date: Sat, 25 Sep 2021 11:54:19 +0200	[thread overview]
Message-ID: <b1b97929-ef62-72ec-609f-bd84c49481d1@amsat.org> (raw)
In-Reply-To: <20210924172527.904294-10-git@xen0n.name>

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.

Maybe name lo12 and hi20?

> +    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?

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

> +    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?

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

> +        }
> +        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;

> +    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?

> +
> +    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;
            }

> +    }
> +
> +    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>