Re: [Qemu-devel] [RFC v2 11/24] riscv: tcg-target: Add the mov and movi instruction

[Richard Henderson]

On 11/27/18 1:08 PM, Alistair Francis wrote:
> +static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd,
> +                         tcg_target_long val)
> +{
> +#if TCG_TARGET_REG_BITS == 64
> +    tcg_target_long lo = sextract64(val, 0, 12);
> +#else
> +    tcg_target_long lo = sextract32(val, 0, 12);
> +#endif

I really think this ifdef should be moved into a helper function.  There are
quite a number of copies of it throughout.

> +    tcg_target_long hi = val - lo;
> +    int shift;
> +    tcg_target_long tmp;
> +
> +    RISCVInsn add32_op = TCG_TARGET_REG_BITS == 64 ? OPC_ADDIW : OPC_ADDI;
> +
> +#if TCG_TARGET_REG_BITS == 64
> +    ptrdiff_t offset = tcg_pcrel_diff(s, (void *)val);
> +#endif
> +
> +    if (TCG_TARGET_REG_BITS == 32 || val == (int32_t)val) {
> +        tcg_out_opc_upper(s, OPC_LUI, rd, hi);
> +        if (lo != 0) {
> +            tcg_out_opc_imm(s, add32_op, rd, rd, lo);
> +        }
> +
> +        return;
> +    }

Your reorg has failed to keep the single insn ADDI(W) case.

> +    /* We can only be here if TCG_TARGET_REG_BITS != 32 */
> +    if (offset == sextract64(offset, 1, 31) << 1) {

The ifdef above won't compile for rv32, because offset is used unconditionally
here.  Remove the ifdef and allow that to be removed as dead code by the 
compiler.

> +        tcg_out_opc_upper(s, OPC_AUIPC, rd, 0);
> +        tcg_out_opc_imm(s, OPC_ADDI, rd, rd, 0);
> +        reloc_call(s->code_ptr - 2, (tcg_insn_unit *)val);
> +        return;
> +    }
> +
> +    shift = ctz64(val);
> +    tmp = val >> shift;
> +
> +    if (tmp == sextract64(tmp, 0, 12)) {
> +        tcg_out_opc_imm(s, OPC_ADDI, rd, TCG_REG_ZERO, 1);

s/1/tmp/

> +        tcg_out_opc_imm(s, OPC_SLLI, rd, rd, ctz64(val));
> +    } else if (!(val >> 31 == 0 || val >> 31 == -1)) {
> +        shift = ctz64(hi);
> +        hi >>= shift;
> +        tcg_out_movi(s, type, rd, hi);
> +        tcg_out_opc_imm(s, OPC_SLLI, rd, rd, shift);
> +        if (lo != 0) {
> +            tcg_out_opc_imm(s, OPC_ADDI, rd, rd, lo);
> +        }
> +    } else {
> +        if (hi != 0) {
> +            tcg_out_opc_upper(s, OPC_LUI, rd, hi);
> +        }
> +        if (lo != 0) {
> +            tcg_out_opc_imm(s, add32_op, rd, hi == 0 ? TCG_REG_ZERO : rd, 
> lo);
> +        }
> +    }

The final else case is identical with the first if.


r~