Re: [PATCH v2 28/37] target/ppc: Move byte-reverse instructions to decodetree

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

On 8/26/26 21:24, Amit Machhiwal wrote:
> On 2026/08/26 10:38 AM, Chinmay Rath wrote:
>> From: Vishal Chourasia <vishalc@linux.ibm.com>
>>
>> Moving the following instructions to decodetree specification:
>>              brd, brw, brh                   : X-form
>>
>> The changes were verified by validating that the tcg ops generated by
>> those instructions remain the same, which were captured with the
>> `-d in_asm,op` flag.
>>
>> This also includes improvements from review feedback:
>> - Add TARGET_PPC64 guards with qemu_build_not_reached() for 32-bit builds
>>
>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>> Reviewed-by: Glenn Miles <milesg@linux.ibm.com>
>> Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
>> ---
>>   target/ppc/insn32.decode                   |  5 +++
>>   target/ppc/translate.c                     | 35 -------------------
>>   target/ppc/translate/fixedpoint-impl.c.inc | 40 ++++++++++++++++++++++
>>   3 files changed, 45 insertions(+), 35 deletions(-)
>>
>> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
>> index f7ab08704f..7614655f72 100644
>> --- a/target/ppc/insn32.decode
>> +++ b/target/ppc/insn32.decode
>> @@ -320,6 +320,11 @@ MFMSR           011111 ..... ----- ----- 0001010011 -   @X_t
>>   MTMSR           011111 ..... ---- . ----- 0010010010 -  @X_rs_l
>>   MTMSRD          011111 ..... ---- . ----- 0010110010 -  @X_rs_l
>>   
>> +### Fixed-Point Byte-Reverse Instructions
>> +BRW             011111 ..... ..... ----- 0010011011 -   @X_sa
>> +BRD             011111 ..... ..... ----- 0010111011 -   @X_sa
>> +BRH             011111 ..... ..... ----- 0011011011 -   @X_sa
>> +
>>   ### Fixed-Point Load Instructions
>>   
>>   LBZ             100010 ..... ..... ................     @D
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 9d29134fa1..8950476be7 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -4532,42 +4532,7 @@ static void gen_dform3D(DisasContext *ctx)
>>       return gen_invalid(ctx);
>>   }
>>   
>> -#if defined(TARGET_PPC64)
>> -/* brd */
>> -static void gen_brd(DisasContext *ctx)
>> -{
>> -    tcg_gen_bswap64_i64(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rS(ctx->opcode)]);
>> -}
>> -
>> -/* brw */
>> -static void gen_brw(DisasContext *ctx)
>> -{
>> -    tcg_gen_bswap64_i64(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rS(ctx->opcode)]);
>> -    tcg_gen_rotli_i64(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rA(ctx->opcode)], 32);
>> -
>> -}
>> -
>> -/* brh */
>> -static void gen_brh(DisasContext *ctx)
>> -{
>> -    TCGv_i64 mask = tcg_constant_i64(0x00ff00ff00ff00ffull);
>> -    TCGv_i64 t1 = tcg_temp_new_i64();
>> -    TCGv_i64 t2 = tcg_temp_new_i64();
>> -
>> -    tcg_gen_shri_i64(t1, cpu_gpr[rS(ctx->opcode)], 8);
>> -    tcg_gen_and_i64(t2, t1, mask);
>> -    tcg_gen_and_i64(t1, cpu_gpr[rS(ctx->opcode)], mask);
>> -    tcg_gen_shli_i64(t1, t1, 8);
>> -    tcg_gen_or_i64(cpu_gpr[rA(ctx->opcode)], t1, t2);
>> -}
>> -#endif
>> -
>>   static opcode_t opcodes[] = {
>> -#if defined(TARGET_PPC64)
>> -GEN_HANDLER_E(brd, 0x1F, 0x1B, 0x05, 0x0000F801, PPC_NONE, PPC2_ISA310),
>> -GEN_HANDLER_E(brw, 0x1F, 0x1B, 0x04, 0x0000F801, PPC_NONE, PPC2_ISA310),
>> -GEN_HANDLER_E(brh, 0x1F, 0x1B, 0x06, 0x0000F801, PPC_NONE, PPC2_ISA310),
>> -#endif
>>   GEN_HANDLER(invalid, 0x00, 0x00, 0x00, 0xFFFFFFFF, PPC_NONE),
>>   GEN_HANDLER_E(copy, 0x1F, 0x06, 0x18, 0x03C00001, PPC_NONE, PPC2_ISA300),
>>   GEN_HANDLER_E(cp_abort, 0x1F, 0x06, 0x1A, 0x03FFF801, PPC_NONE, PPC2_ISA300),
>> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
>> index a75279022c..cef67d6431 100644
>> --- a/target/ppc/translate/fixedpoint-impl.c.inc
>> +++ b/target/ppc/translate/fixedpoint-impl.c.inc
>> @@ -17,6 +17,46 @@
>>    * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>    */
>>   
>> +/*
>> + * Byte-Reverse Instructions
>> + */
>> +static bool do_byte_reverse_X_sa(DisasContext *ctx, arg_X_sa *a, int kind)
>> +{
>> +#if defined(TARGET_PPC64)
>> +    switch (kind) {
>> +    case 0: /* brd */
>> +        tcg_gen_bswap64_i64(cpu_gpr[a->ra], cpu_gpr[a->rs]);
>> +        break;
>> +    case 1: /* brw */
>> +        tcg_gen_bswap64_i64(cpu_gpr[a->ra], cpu_gpr[a->rs]);
>> +        tcg_gen_rotli_i64(cpu_gpr[a->ra], cpu_gpr[a->ra], 32);
>> +        break;
>> +    case 2: /* brh */
>> +    {
>> +        TCGv_i64 mask = tcg_constant_i64(0x00ff00ff00ff00ffull);
>> +        TCGv_i64 t1 = tcg_temp_new_i64();
>> +        TCGv_i64 t2 = tcg_temp_new_i64();
>> +
>> +        tcg_gen_shri_i64(t1, cpu_gpr[a->rs], 8);
>> +        tcg_gen_and_i64(t2, t1, mask);
>> +        tcg_gen_and_i64(t1, cpu_gpr[a->rs], mask);
>> +        tcg_gen_shli_i64(t1, t1, 8);
>> +        tcg_gen_or_i64(cpu_gpr[a->ra], t1, t2);
>> +        break;
>> +    }
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +#else
>> +    qemu_build_not_reached();
>> +#endif
>> +    return true;
>> +}
>> +
>> +TRANS64_FLAGS2(ISA310, BRD, do_byte_reverse_X_sa, 0)
>> +TRANS64_FLAGS2(ISA310, BRW, do_byte_reverse_X_sa, 1)
>> +TRANS64_FLAGS2(ISA310, BRH, do_byte_reverse_X_sa, 2)
> The kind values 0, 1, 2 are opaque at the call site — a reader has to
> cross-reference the switch inside do_byte_reverse_X_sa to know which is
> which. The more common pattern in this series is three small separate
> translators, which avoids the switch entirely and is self-documenting:
>
> static bool trans_BRD(DisasContext *ctx, arg_BRD *a)
> {
>      REQUIRE_64BIT(ctx);
>      REQUIRE_INSNS_FLAGS2(ctx, ISA310);
>      tcg_gen_bswap64_i64(cpu_gpr[a->ra], cpu_gpr[a->rs]);
>      return true;
> }
>
> ...and similarly for BRW and BRH. This also eliminates the
> do_byte_reverse_X_sa wrapper and the #else qemu_build_not_reached()
> block entirely, since REQUIRE_64BIT already handles 32-bit builds.
Separating the trans_XXX handlers as suggested but preserving #if 
defined (TARGET_PPC64) and #else qemu_build_not_reached() blocks in 
them, since REQUIRE_64B is a run time check. #if defined (TARGET_PPC64) 
is required during compilation.
>
> ~Amit
>