On 2026/08/26 10:38 AM, Chinmay Rath wrote:
> From: Vishal Chourasia <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Glenn Miles <[email protected]>
> Signed-off-by: Chinmay Rath <[email protected]>
> ---
> 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.
~Amit
