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

[Chinmay Rath]

On 8/26/26 21:24, Amit Machhiwal wrote:
On 2026/08/26 10:38 AM, Chinmay Rath wrote:
From: Vishal Chourasia <vish...@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 <vish...@linux.ibm.com>
Reviewed-by: Glenn Miles <mil...@linux.ibm.com>
Signed-off-by: Chinmay Rath <ra...@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