diff mbox series

[RESEND,04/10] target/ppc: Move mffsce to decodetree

Message ID 20220517164744.58131-5-victor.colombo@eldorado.org.br
State New
Headers show
Series BCDA and mffscdrn implementations | expand

Commit Message

Víctor Colombo May 17, 2022, 4:47 p.m. UTC 
Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
---
 target/ppc/insn32.decode           |  1 +
 target/ppc/translate/fp-impl.c.inc | 45 +++++++++++-------------------
 target/ppc/translate/fp-ops.c.inc  |  2 --
 3 files changed, 18 insertions(+), 30 deletions(-)

Comments

Richard Henderson May 17, 2022, 6:49 p.m. UTC | #1 
On 5/17/22 09:47, Víctor Colombo wrote:
> -static void do_mffsc(int rt, uint64_t set_mask)
> +static void do_mffsc(int rt, TCGv_i64 t1, uint64_t set_mask,
> +                     uint64_t clear_mask, uint32_t fpscr_mask)
>   {
>       TCGv_i64 fpscr;
>   
> @@ -618,6 +619,12 @@ static void do_mffsc(int rt, uint64_t set_mask)
>       tcg_gen_andi_i64(fpscr, fpscr, set_mask);
>       set_fpr(rt, fpscr);
>   
> +    if (fpscr_mask) {
> +        tcg_gen_andi_i64(fpscr, fpscr, clear_mask);

The naming doesn't seem right for clear_mask.  I would expect clear_mask to contain the 
bits that we want to remove, and the computation to be

    fpscr &= ~clear_mask

> +        tcg_gen_or_i64(fpscr, fpscr, t1);

Can we get a better name than t1?  Also, I think perhaps NULL would be better for when we 
do not wish to include this.  I suppose tcg_constant_i64(0) is *probably* already in the 
constant hash table, but why look it up at all?

> +        gen_helper_store_fpscr(cpu_env, fpscr, tcg_constant_i32(fpscr_mask));

Given than everything in here is fpscr related, perhaps "store_mask" is a better name?

Also, this is the second time we're modifying the new do_mffsc function.  Does it actually 
make more sense to reverse the order of these patches so that MFFSCRN comes first, as that 
is the one that takes the most complex form of do_mffsc.

Alternately, does it really make sense to try to do everything in one function, with 4 
extra parameters that turn out to be very specific to single instructions?  E.g. the t1 
parameter would probably be better implemented with a deposit of the RN field, rather than 
separate ANDs and OR.


r~
diff mbox series

Patch

diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index 2cd5603353..a8535e5684 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -325,6 +325,7 @@  SETNBCR         011111 ..... ..... ----- 0111100000 -   @X_bi
 
 MFFS            111111 ..... 00000 ----- 1001000111 .   @X_t_rc
 MFFSL           111111 ..... 11000 ----- 1001000111 -   @X_t
+MFFSCE          111111 ..... 00001 ----- 1001000111 -   @X_t
 
 ### Decimal Floating-Point Arithmetic Instructions
 
diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
index 22b0605e24..4520edc439 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -607,7 +607,8 @@  static void gen_mffs(DisasContext *ctx)
     tcg_temp_free_i64(t0);
 }
 
-static void do_mffsc(int rt, uint64_t set_mask)
+static void do_mffsc(int rt, TCGv_i64 t1, uint64_t set_mask,
+                     uint64_t clear_mask, uint32_t fpscr_mask)
 {
     TCGv_i64 fpscr;
 
@@ -618,6 +619,12 @@  static void do_mffsc(int rt, uint64_t set_mask)
     tcg_gen_andi_i64(fpscr, fpscr, set_mask);
     set_fpr(rt, fpscr);
 
+    if (fpscr_mask) {
+        tcg_gen_andi_i64(fpscr, fpscr, clear_mask);
+        tcg_gen_or_i64(fpscr, fpscr, t1);
+        gen_helper_store_fpscr(cpu_env, fpscr, tcg_constant_i32(fpscr_mask));
+    }
+
     tcg_temp_free_i64(fpscr);
 }
 
@@ -625,7 +632,7 @@  static bool trans_MFFS(DisasContext *ctx, arg_X_t_rc *a)
 {
     REQUIRE_FPU(ctx);
 
-    do_mffsc(a->rt, 0xFFFFFFFFFFFFFFFFULL);
+    do_mffsc(a->rt, tcg_constant_i64(0), 0xFFFFFFFFFFFFFFFFULL, 0, 0);
     if (a->rc) {
         gen_set_cr1_from_fpscr(ctx);
     }
@@ -638,39 +645,21 @@  static bool trans_MFFSL(DisasContext *ctx, arg_X_t *a)
     REQUIRE_INSNS_FLAGS2(ctx, ISA300);
     REQUIRE_FPU(ctx);
 
-    do_mffsc(a->rt, FP_DRN | FP_STATUS | FP_ENABLES | FP_NI | FP_RN);
+    do_mffsc(a->rt, tcg_constant_i64(0),
+        FP_DRN | FP_STATUS | FP_ENABLES | FP_NI | FP_RN, 0, 0);
 
     return true;
 }
 
-/* mffsce */
-static void gen_mffsce(DisasContext *ctx)
+static bool trans_MFFSCE(DisasContext *ctx, arg_X_t *a)
 {
-    TCGv_i64 t0;
-    TCGv_i32 mask;
-
-    if (unlikely(!(ctx->insns_flags2 & PPC2_ISA300))) {
-        return gen_mffs(ctx);
-    }
-
-    if (unlikely(!ctx->fpu_enabled)) {
-        gen_exception(ctx, POWERPC_EXCP_FPU);
-        return;
-    }
-
-    t0 = tcg_temp_new_i64();
-
-    gen_reset_fpstatus();
-    tcg_gen_extu_tl_i64(t0, cpu_fpscr);
-    set_fpr(rD(ctx->opcode), t0);
+    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+    REQUIRE_FPU(ctx);
 
-    /* Clear exception enable bits in the FPSCR.  */
-    tcg_gen_andi_i64(t0, t0, ~FP_ENABLES);
-    mask = tcg_const_i32(0x0003);
-    gen_helper_store_fpscr(cpu_env, t0, mask);
+    do_mffsc(a->rt, tcg_constant_i64(0), 0xFFFFFFFFFFFFFFFFULL,
+             ~FP_ENABLES, 0x0003);
 
-    tcg_temp_free_i32(mask);
-    tcg_temp_free_i64(t0);
+    return true;
 }
 
 static void gen_helper_mffscrn(DisasContext *ctx, TCGv_i64 t1)
diff --git a/target/ppc/translate/fp-ops.c.inc b/target/ppc/translate/fp-ops.c.inc
index fe7dd1d1bb..46357a3c4c 100644
--- a/target/ppc/translate/fp-ops.c.inc
+++ b/target/ppc/translate/fp-ops.c.inc
@@ -75,8 +75,6 @@  GEN_HANDLER_E(fcpsgn, 0x3F, 0x08, 0x00, 0x00000000, PPC_NONE, PPC2_ISA205),
 GEN_HANDLER_E(fmrgew, 0x3F, 0x06, 0x1E, 0x00000001, PPC_NONE, PPC2_VSX207),
 GEN_HANDLER_E(fmrgow, 0x3F, 0x06, 0x1A, 0x00000001, PPC_NONE, PPC2_VSX207),
 GEN_HANDLER(mcrfs, 0x3F, 0x00, 0x02, 0x0063F801, PPC_FLOAT),
-GEN_HANDLER_E_2(mffsce, 0x3F, 0x07, 0x12, 0x01, 0x00000000, PPC_FLOAT,
-    PPC2_ISA300),
 GEN_HANDLER_E_2(mffscrn, 0x3F, 0x07, 0x12, 0x16, 0x00000000, PPC_FLOAT,
     PPC_NONE),
 GEN_HANDLER_E_2(mffscrni, 0x3F, 0x07, 0x12, 0x17, 0x00000000, PPC_FLOAT,