Re: [PATCH 7/8] target/ppc: Move cmp{rb, eqb}, tw[i], td[i], isel instructions to decodetree.

[Chinmay Rath <rathc@linux.vnet.ibm.com>]

Hi Richard,

On 4/17/24 00:50, Richard Henderson wrote:
> On 4/15/24 23:39, Chinmay Rath wrote:
>> Moving the following instructions to decodetree specification :
>>
>>     cmp{rb, eqb}, t{w, d}    : X-form
>>     t{w, d}i        : D-form
>>     isel            : A-form
>>
>> The changes were verified by validating that the tcg ops generated by 
>> those
>> instructions remain the same, which were captured using the '-d 
>> in_asm,op' flag.
>>
>> Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
>
> A faithful reorg of the existing code, so,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
Thank you.
> Notes for improvement:
>
>> +static bool trans_CMPRB(DisasContext *ctx, arg_CMPRB *a)
>> +{
>> +    TCGv_i32 src1 = tcg_temp_new_i32();
>> +    TCGv_i32 src2 = tcg_temp_new_i32();
>> +    TCGv_i32 src2lo = tcg_temp_new_i32();
>> +    TCGv_i32 src2hi = tcg_temp_new_i32();
>> +    TCGv_i32 crf = cpu_crf[a->bf];
>> +
>> +    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
>> +    tcg_gen_trunc_tl_i32(src1, cpu_gpr[a->ra]);
>> +    tcg_gen_trunc_tl_i32(src2, cpu_gpr[a->rb]);
>> +
>> +    tcg_gen_andi_i32(src1, src1, 0xFF);
>> +    tcg_gen_ext8u_i32(src2lo, src2);
>> +    tcg_gen_shri_i32(src2, src2, 8);
>> +    tcg_gen_ext8u_i32(src2hi, src2);
>
> tcg_gen_extract_i32(src2hi, src2, 8, 8);
>
>> +
>> +    tcg_gen_setcond_i32(TCG_COND_LEU, src2lo, src2lo, src1);
>> +    tcg_gen_setcond_i32(TCG_COND_LEU, src2hi, src1, src2hi);
>> +    tcg_gen_and_i32(crf, src2lo, src2hi);
>> +
>> +    if (a->l) {
>> +        tcg_gen_shri_i32(src2, src2, 8);
>> +        tcg_gen_ext8u_i32(src2lo, src2);
>
> tcg_gen_extract_i32(src2lo, src2, 16, 8);
>
>> +        tcg_gen_shri_i32(src2, src2, 8);
>> +        tcg_gen_ext8u_i32(src2hi, src2);
>
> tcg_gen_extract_i32(src2hi, src2, 24, 8);
>
Will update the above in v2.

Will implement the below improvements for trap insns as a separate patch 
later.

>> +/*
>> + * Fixed-Point Trap Instructions
>> + */
>> +
>> +static bool trans_TW(DisasContext *ctx, arg_TW *a)
>> +{
>> +    TCGv_i32 t0;
>> +
>> +    if (check_unconditional_trap(ctx, a->rt)) {
>> +        return true;
>> +    }
>> +    t0 = tcg_constant_i32(a->rt);
>> +    gen_helper_TW(tcg_env, cpu_gpr[a->ra], cpu_gpr[a->rb], t0);
>> +    return true;
>> +}
>> +
>> +static bool trans_TWI(DisasContext *ctx, arg_TWI *a)
>> +{
>> +    TCGv t0;
>> +    TCGv_i32 t1;
>> +
>> +    if (check_unconditional_trap(ctx, a->rt)) {
>> +        return true;
>> +    }
>> +    t0 = tcg_constant_tl(a->si);
>> +    t1 = tcg_constant_i32(a->rt);
>> +    gen_helper_TW(tcg_env, cpu_gpr[a->ra], t0, t1);
>> +    return true;
>> +}
>> +
>> +static bool trans_TD(DisasContext *ctx, arg_TD *a)
>> +{
>> +    TCGv_i32 t0;
>> +
>> +    REQUIRE_64BIT(ctx);
>> +    if (check_unconditional_trap(ctx, a->rt)) {
>> +        return true;
>> +    }
>> +    t0 = tcg_constant_i32(a->rt);
>> +    gen_helper_TD(tcg_env, cpu_gpr[a->ra], cpu_gpr[a->rb], t0);
>> +    return true;
>> +}
>> +
>> +static bool trans_TDI(DisasContext *ctx, arg_TDI *a)
>> +{
>> +    TCGv t0;
>> +    TCGv_i32 t1;
>> +
>> +    REQUIRE_64BIT(ctx);
>> +    if (check_unconditional_trap(ctx, a->rt)) {
>> +        return true;
>> +    }
>> +    t0 = tcg_constant_tl(a->si);
>> +    t1 = tcg_constant_i32(a->rt);
>> +    gen_helper_TD(tcg_env, cpu_gpr[a->ra], t0, t1);
>> +    return true;
>> +}
>
> See target/sparc/translate.c, delay_exception, for a method of 
> implementing compare-and-trap inline with no inline branch penalty.
>
> static void do_conditional_trap(DisasContext *ctx, unsigned to, TCGv 
> a, TCGv b)
> {
>     static const TCGCond ucond[8] = {
>         TCG_COND_NEVER, TCG_COND_GTU, TCG_COND_LTU, TCG_COND_NE,
>         TCG_COND_EQ,    TCG_COND_GEU, TCG_COND_LEU, TCG_COND_ALWAYS,
>     };
>     static const TCGCond scond[8] = {
>         TCG_COND_NEVER, TCG_COND_EQ,  TCG_COND_GT,  TCG_COND_GE,
>         TCG_COND_LT,    TCG_COND_LE,  TCG_COND_NE, TCG_COND_ALWAYS,
>     };
>
>     TCGCond uc = ucond[to & 7];
>     TCGCond sc = scond[to >> 2];
>
>     /* There is overlap with EQ; we may not need both comparisons. */
>     if (!(to & 0x18)) {
>         sc = TCG_COND_NEVER;
>     } else if (!(to & 0x03)) {
>         uc = TCG_COND_NEVER;
>     }
>
>     if (uc == TCG_COND_ALWAYS || sc == TCG_COND_ALWAYS) {
>         unconditional trap;
>         return true;
>     }
>     if (uc == TCG_COND_NEVER && sc == TCG_COND_NEVER) {
>         return true;
>     }
>
>     e = delay_exception(ctx, POWERPC_EXCP_TRAP);
>
>     if (uc != TCG_COND_NEVER) {
>         tcg_gen_brcond_tl(uc, a, b, e->lab);
>     }
>     if (sc != TCG_COND_NEVER) {
>         tcg_gen_brcond_tl(sc, a, b, e->lab);
>     }
>     return true;
> }
>
> bool trans_TW(...)
> {
>     TCGv a = tcg_temp_new();
>     TCGv b = tcg_temp_new();
>
>     /* Note that consistent sign extensions work for unsigned 
> comparisons. */
>     tcg_gen_exts_i32_tl(a, ra);
>     tcg_gen_exts_i32_tl(b, rb);
>     return do_conditional_trap(ctx, to, a, b);
> }
>
> etc.
>
>
Thanks,
Chinmay
> r~