Re: [PATCH 5/5] target/ppc: Implement paddi and replace addi insns

[Richard Henderson]

On 4/13/21 2:11 PM, Luis Pires wrote:
> +++ b/target/ppc/translate/fixedpoint-impl.c.inc
> @@ -0,0 +1,26 @@

Missing copyright+license header.

> +static bool trans_paddi(DisasContext *ctx, arg_paddi *a)
> +{
> +    if (a->r == 0) {
> +        if (a->ra == 0) {
> +            /* li case */
> +            tcg_gen_movi_tl(cpu_gpr[a->rt], a->si);
> +        } else {
> +            tcg_gen_addi_tl(cpu_gpr[a->rt],
> +                            cpu_gpr[a->ra], a->si);
> +        }
> +    } else {
> +        if (a->ra == 0) {
> +            tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_nip, a->si);
> +        } else {
> +            /* invalid form */
> +            gen_invalid(ctx);
> +        }
> +    }
> +
> +    return true;
> +}
> +
> +static bool trans_addi(DisasContext *ctx, arg_addi *a)
> +{
> +    return trans_paddi(ctx, a);
> +}

Just a note about decodetree: this kind of thing is where you would use the 
same name for both patterns, so that you would not need to have a separate 
symbol for addi (or vice versa).

That said, I'm now having a bit of a read-up on power10, and I have some 
suggestions.

First, type 2 and type 3 prefixes modify existing instructions.  Which means 
that you are going to wind up with a lot of duplication.  Preferentially, we 
should avoid that.

One example of how to approach this is target/microblaze, which has an "imm" 
instruction prefix to extend a 16-bit immediate to a 32-bit immediate.  This 
can be worked into decodetree directly:

# Include any IMM prefix in the value reported.
%extimm         0:s16 !function=typeb_imm
@typeb          ...... rd:5 ra:5 ................ \
                &typeb imm=%extimm

static int typeb_imm(DisasContext *dc, int x)
{
    if (dc->tb_flags & IMM_FLAG) {
        return deposit32(dc->ext_imm, 0, 16, x);
    }
    return x;
}

static bool trans_imm(DisasContext *dc, arg_imm *arg)
{
    if (invalid_delay_slot(dc, "imm")) {
        return true;
    }
    dc->ext_imm = arg->imm << 16;
    tcg_gen_movi_i32(cpu_imm, dc->ext_imm);
    dc->tb_flags_to_set = IMM_FLAG;
    return true;
}

We decode "imm" as a separate instruction, set some bits in DisasContext, and 
then use those bits while decoding the next instruction.

I think the exact mechanism that microblaze uses is going to be too simplistic 
for Power10, but the basic idea of modifying the "normal" instructions is still 
sound, I think.

Using addi+paddi as an example, what about

# All ppc formats have names -- use them.
%MLS        r ie
prefix_MLS  000001 10 -- r:1 -- ie:s18          &MLS

# TODO: decodetree extension -- allow :type after name.
# The SI field needs to be 64-bit for MLS:D-form.
%D          rt ra si:int64_t
@D          ...... rt:5 ra:5 si:s16

ADDI        001110 ..... ..... ................ @D


static bool
trans_prefix_MLS(DisasContext *ctx, arg_MLS *a)
{
    if (cpu does not support prefixes ||
        ctx->prefix_type != PREFIX_NONE) {
        return false;
    }
    /* Record the prefix for the next instruction. */
    ctx->prefix_type = PREFIX_MLS;
    ctx->prefix_data.mls = *a;
    return true;
}

static bool
allow_prefix_MLS(DisasContext *ctx, arg_D *a)
{
    int64_t imm;

    /* Require MLS prefix or no prefix. */
    if (ctx->prefix_type != PREFIX_MLS) {
        if (ctx->prefix_type == PREFIX_NONE) {
            return true;
        }
        gen_invalid(ctx);
        return false;
    }

    /*
     * Concatenate the two immediate fields.
     * Note that IE is sign-extended 18 bits,
     * so this forms a signed 34-bit constant.
     */
    imm = deposit64(a->si, 16, 48, ctx->prefix_data.mls.ie);

    /*
     * If R, then the constant is pc-relative,
     * and RA must be 0.
     */
    if (ctx->prefix_data.mls.r) {
        if (ctx->prefix_data.mls.ra != 0) {
            gen_invalid(ctx);
            return false;
        }
        imm += ctx->cia;
    }
    a->si = imm;
    return true;
}

static bool
trans_ADDI(DisasContext *ctx, arg_D *a)
{
    if (!allow_prefix_MLS(ctx, a)) {
        return true;
    }
    if (a->ra) {
        tcg_gen_addi_tl(cpu_gpr[a->rt],
                        cpu_gpr[a->ra], a->si);
    } else {
        tcg_gen_movi_tl(cpu_gpr[a->rt], a->si);
    }
    return true;
}

This approach seems like it will work fine for MLS and MMIR prefixes.  For 8LS, 
8RR, and MRR prefixes, we'll need some extra help within ppc_tr_translate_insn. 
 E.g.

    insn = translator_ldl_swap(env, ctx->base.pc_next,
                               need_byteswap(ctx));
    switch (ctx->prefix_type) {
    case PREFIX_NONE:
        ok = decode_opcode_space_0(ctx, insn) ||
             decode_legacy(ctx, insn);
        break;
    case PREFIX_MLS:
    case PREFIX_MMIRR:
        ok = decode_opcode_space_0(ctx, insn);
        break;
    case PREFIX_8LS:
    case PREFIX_8RR:
        ok = decode_opcode_space_1(ctx, insn);
        break;
    case PREFIX_MRR:
        /*
         * The only instruction with this prefix is PNOP.
         * TODO: diagnose the set of patterns that are illegal:
         * branches, rfebb, sync other than isync, or a
         * service processor attention.
         * The Engineering Note allows us to either diagnose
         * these as illegal, or treat them all as no-op.
         */
        ok = true;
        break;
    default:
        g_assert_not_reached();
    }
    if (!ok) {
        gen_invalid(ctx);
    }


r~