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

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

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~