[PATCH 0/3] target/ppc: Move fsqrt[s] to decodetree

[PATCH 0/3] target/ppc: Move fsqrt[s] to decodetree

[Víctor Colombo]

Move fsqrt and fsqrts instructions from decode legacy to decodetree.

Víctor Colombo (3):
  target/ppc: Move fsqrt to decodetree
  target/ppc: Move fsqrts to decodetree
  target/ppc: Merge fsqrt and fsqrts helpers

 target/ppc/fpu_helper.c            | 35 ++++++++-------------
 target/ppc/helper.h                |  4 +--
 target/ppc/insn32.decode           |  8 +++++
 target/ppc/translate/fp-impl.c.inc | 50 ++++++++++--------------------
 target/ppc/translate/fp-ops.c.inc  |  2 --
 5 files changed, 40 insertions(+), 59 deletions(-)

-- 
2.25.1

[PATCH 1/3] target/ppc: Move fsqrt to decodetree

[Víctor Colombo]

Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
---
 target/ppc/insn32.decode           |  7 +++++++
 target/ppc/translate/fp-impl.c.inc | 29 +++++++++++++++++------------
 target/ppc/translate/fp-ops.c.inc  |  1 -
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index eb41efc100..b55d1550f3 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -20,6 +20,9 @@
 &A              frt fra frb frc rc:bool
 @A              ...... frt:5 fra:5 frb:5 frc:5 ..... rc:1       &A
 
+&A_tb           frt frb rc:bool
+@A_tb           ...... frt:5 ..... frb:5 ..... ..... rc:1       &A_tb
+
 &D              rt ra si:int64_t
 @D              ...... rt:5 ra:5 si:s16                         &D
 
@@ -353,6 +356,10 @@ STFDU           110111 ..... ...... ...............     @D
 STFDX           011111 ..... ...... .... 1011010111 -   @X
 STFDUX          011111 ..... ...... .... 1011110111 -   @X
 
+### Floating-Point Arithmetic Instructions
+
+FSQRT           111111 ..... ----- ..... ----- 10110 .  @A_tb
+
 ### Floating-Point Select Instruction
 
 FSEL            111111 ..... ..... ..... ..... 10111 .  @A
diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
index 0e893eafa7..e8359af005 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -254,29 +254,34 @@ static bool trans_FSEL(DisasContext *ctx, arg_A *a)
 GEN_FLOAT_AB(sub, 0x14, 0x000007C0, 1, PPC_FLOAT);
 /* Optional: */
 
-/* fsqrt */
-static void gen_fsqrt(DisasContext *ctx)
+static bool do_helper_fsqrt(DisasContext *ctx, arg_A_tb *a,
+                            void (*helper)(TCGv_i64, TCGv_ptr, TCGv_i64))
 {
-    TCGv_i64 t0;
-    TCGv_i64 t1;
-    if (unlikely(!ctx->fpu_enabled)) {
-        gen_exception(ctx, POWERPC_EXCP_FPU);
-        return;
-    }
+    TCGv_i64 t0, t1;
+
+    REQUIRE_INSNS_FLAGS(ctx, FLOAT_FSQRT);
+    REQUIRE_FPU(ctx);
+
     t0 = tcg_temp_new_i64();
     t1 = tcg_temp_new_i64();
+
     gen_reset_fpstatus();
-    get_fpr(t0, rB(ctx->opcode));
-    gen_helper_fsqrt(t1, cpu_env, t0);
-    set_fpr(rD(ctx->opcode), t1);
+    get_fpr(t0, a->frb);
+    helper(t1, cpu_env, t0);
+    set_fpr(a->frt, t1);
     gen_compute_fprf_float64(t1);
-    if (unlikely(Rc(ctx->opcode) != 0)) {
+    if (unlikely(a->rc != 0)) {
         gen_set_cr1_from_fpscr(ctx);
     }
+
     tcg_temp_free_i64(t0);
     tcg_temp_free_i64(t1);
+
+    return true;
 }
 
+TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);
+
 static void gen_fsqrts(DisasContext *ctx)
 {
     TCGv_i64 t0;
diff --git a/target/ppc/translate/fp-ops.c.inc b/target/ppc/translate/fp-ops.c.inc
index 1b65f5ab73..38759f5939 100644
--- a/target/ppc/translate/fp-ops.c.inc
+++ b/target/ppc/translate/fp-ops.c.inc
@@ -62,7 +62,6 @@ GEN_HANDLER_E(stfdepx, 0x1F, 0x1F, 0x16, 0x00000001, PPC_NONE, PPC2_BOOKE206),
 GEN_HANDLER_E(stfdpx, 0x1F, 0x17, 0x1C, 0x00200001, PPC_NONE, PPC2_ISA205),
 
 GEN_HANDLER(frsqrtes, 0x3B, 0x1A, 0xFF, 0x001F07C0, PPC_FLOAT_FRSQRTES),
-GEN_HANDLER(fsqrt, 0x3F, 0x16, 0xFF, 0x001F07C0, PPC_FLOAT_FSQRT),
 GEN_HANDLER(fsqrts, 0x3B, 0x16, 0xFF, 0x001F07C0, PPC_FLOAT_FSQRT),
 GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT),
 GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT),
-- 
2.25.1

[PATCH 2/3] target/ppc: Move fsqrts to decodetree

[Víctor Colombo]

Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
---
 target/ppc/insn32.decode           |  1 +
 target/ppc/translate/fp-impl.c.inc | 23 +----------------------
 target/ppc/translate/fp-ops.c.inc  |  1 -
 3 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index b55d1550f3..7eb808a43f 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -359,6 +359,7 @@ STFDUX          011111 ..... ...... .... 1011110111 -   @X
 ### Floating-Point Arithmetic Instructions
 
 FSQRT           111111 ..... ----- ..... ----- 10110 .  @A_tb
+FSQRTS          111011 ..... ----- ..... ----- 10110 .  @A_tb
 
 ### Floating-Point Select Instruction
 
diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
index e8359af005..7a90c0e350 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -281,28 +281,7 @@ static bool do_helper_fsqrt(DisasContext *ctx, arg_A_tb *a,
 }
 
 TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);
-
-static void gen_fsqrts(DisasContext *ctx)
-{
-    TCGv_i64 t0;
-    TCGv_i64 t1;
-    if (unlikely(!ctx->fpu_enabled)) {
-        gen_exception(ctx, POWERPC_EXCP_FPU);
-        return;
-    }
-    t0 = tcg_temp_new_i64();
-    t1 = tcg_temp_new_i64();
-    gen_reset_fpstatus();
-    get_fpr(t0, rB(ctx->opcode));
-    gen_helper_fsqrts(t1, cpu_env, t0);
-    set_fpr(rD(ctx->opcode), t1);
-    gen_compute_fprf_float64(t1);
-    if (unlikely(Rc(ctx->opcode) != 0)) {
-        gen_set_cr1_from_fpscr(ctx);
-    }
-    tcg_temp_free_i64(t0);
-    tcg_temp_free_i64(t1);
-}
+TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts);
 
 /***                     Floating-Point multiply-and-add                   ***/
 /* fmadd - fmadds */
diff --git a/target/ppc/translate/fp-ops.c.inc b/target/ppc/translate/fp-ops.c.inc
index 38759f5939..d4c6c4bed1 100644
--- a/target/ppc/translate/fp-ops.c.inc
+++ b/target/ppc/translate/fp-ops.c.inc
@@ -62,7 +62,6 @@ GEN_HANDLER_E(stfdepx, 0x1F, 0x1F, 0x16, 0x00000001, PPC_NONE, PPC2_BOOKE206),
 GEN_HANDLER_E(stfdpx, 0x1F, 0x17, 0x1C, 0x00200001, PPC_NONE, PPC2_ISA205),
 
 GEN_HANDLER(frsqrtes, 0x3B, 0x1A, 0xFF, 0x001F07C0, PPC_FLOAT_FRSQRTES),
-GEN_HANDLER(fsqrts, 0x3B, 0x16, 0xFF, 0x001F07C0, PPC_FLOAT_FSQRT),
 GEN_HANDLER(fcmpo, 0x3F, 0x00, 0x01, 0x00600001, PPC_FLOAT),
 GEN_HANDLER(fcmpu, 0x3F, 0x00, 0x00, 0x00600001, PPC_FLOAT),
 GEN_HANDLER(fabs, 0x3F, 0x08, 0x08, 0x001F0000, PPC_FLOAT),
-- 
2.25.1

[PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Víctor Colombo]

These two helpers are almost identical, differing only by the softfloat
operation it calls. Merge them into one using a macro.
Also, take this opportunity to capitalize the helper name as we moved
the instruction to decodetree in a previous patch.

Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
---
 target/ppc/fpu_helper.c            | 35 +++++++++++-------------------
 target/ppc/helper.h                |  4 ++--
 target/ppc/translate/fp-impl.c.inc |  4 ++--
 3 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 0f045b70f8..32995179b5 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState *env, int flags,
     }
 }
 
-/* fsqrt - fsqrt. */
-float64 helper_fsqrt(CPUPPCState *env, float64 arg)
-{
-    float64 ret = float64_sqrt(arg, &env->fp_status);
-    int flags = get_float_exception_flags(&env->fp_status);
-
-    if (unlikely(flags & float_flag_invalid)) {
-        float_invalid_op_sqrt(env, flags, 1, GETPC());
-    }
-
-    return ret;
+#define FPU_FSQRT(name, op)                                                   \
+float64 helper_##name(CPUPPCState *env, float64 arg)                          \
+{                                                                             \
+    float64 ret = op(arg, &env->fp_status);                                   \
+    int flags = get_float_exception_flags(&env->fp_status);                   \
+                                                                              \
+    if (unlikely(flags & float_flag_invalid)) {                               \
+        float_invalid_op_sqrt(env, flags, 1, GETPC());                        \
+    }                                                                         \
+                                                                              \
+    return ret;                                                               \
 }
 
-/* fsqrts - fsqrts. */
-float64 helper_fsqrts(CPUPPCState *env, float64 arg)
-{
-    float64 ret = float64r32_sqrt(arg, &env->fp_status);
-    int flags = get_float_exception_flags(&env->fp_status);
-
-    if (unlikely(flags & float_flag_invalid)) {
-        float_invalid_op_sqrt(env, flags, 1, GETPC());
-    }
-    return ret;
-}
+FPU_FSQRT(FSQRT, float64_sqrt)
+FPU_FSQRT(FSQRTS, float64r32_sqrt)
 
 /* fre - fre. */
 float64 helper_fre(CPUPPCState *env, float64 arg)
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 159b352f6e..68610896b8 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64)
 DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64)
 DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64)
 DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64)
-DEF_HELPER_2(fsqrt, f64, env, f64)
-DEF_HELPER_2(fsqrts, f64, env, f64)
+DEF_HELPER_2(FSQRT, f64, env, f64)
+DEF_HELPER_2(FSQRTS, f64, env, f64)
 DEF_HELPER_2(fre, i64, env, i64)
 DEF_HELPER_2(fres, i64, env, i64)
 DEF_HELPER_2(frsqrte, i64, env, i64)
diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
index 7a90c0e350..8d5cf0f982 100644
--- a/target/ppc/translate/fp-impl.c.inc
+++ b/target/ppc/translate/fp-impl.c.inc
@@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, arg_A_tb *a,
     return true;
 }
 
-TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);
-TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts);
+TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT);
+TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS);
 
 /***                     Floating-Point multiply-and-add                   ***/
 /* fmadd - fmadds */
-- 
2.25.1

Re: [PATCH 1/3] target/ppc: Move fsqrt to decodetree

[Richard Henderson]

On 9/5/22 13:37, Víctor Colombo wrote:
> Signed-off-by: Víctor Colombo<victor.colombo@eldorado.org.br>
> ---
>   target/ppc/insn32.decode           |  7 +++++++
>   target/ppc/translate/fp-impl.c.inc | 29 +++++++++++++++++------------
>   target/ppc/translate/fp-ops.c.inc  |  1 -
>   3 files changed, 24 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/3] target/ppc: Move fsqrts to decodetree

[Richard Henderson]

On 9/5/22 13:37, Víctor Colombo wrote:
> Signed-off-by: Víctor Colombo<victor.colombo@eldorado.org.br>
> ---
>   target/ppc/insn32.decode           |  1 +
>   target/ppc/translate/fp-impl.c.inc | 23 +----------------------
>   target/ppc/translate/fp-ops.c.inc  |  1 -
>   3 files changed, 2 insertions(+), 23 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Richard Henderson]

On 9/5/22 13:37, Víctor Colombo wrote:
> These two helpers are almost identical, differing only by the softfloat
> operation it calls. Merge them into one using a macro.
> Also, take this opportunity to capitalize the helper name as we moved
> the instruction to decodetree in a previous patch.
> 
> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
> ---
>   target/ppc/fpu_helper.c            | 35 +++++++++++-------------------
>   target/ppc/helper.h                |  4 ++--
>   target/ppc/translate/fp-impl.c.inc |  4 ++--
>   3 files changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 0f045b70f8..32995179b5 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState *env, int flags,
>       }
>   }
>   
> -/* fsqrt - fsqrt. */
> -float64 helper_fsqrt(CPUPPCState *env, float64 arg)
> -{
> -    float64 ret = float64_sqrt(arg, &env->fp_status);
> -    int flags = get_float_exception_flags(&env->fp_status);
> -
> -    if (unlikely(flags & float_flag_invalid)) {
> -        float_invalid_op_sqrt(env, flags, 1, GETPC());
> -    }
> -
> -    return ret;
> +#define FPU_FSQRT(name, op)                                                   \
> +float64 helper_##name(CPUPPCState *env, float64 arg)                          \
> +{                                                                             \
> +    float64 ret = op(arg, &env->fp_status);                                   \
> +    int flags = get_float_exception_flags(&env->fp_status);                   \
> +                                                                              \
> +    if (unlikely(flags & float_flag_invalid)) {                               \
> +        float_invalid_op_sqrt(env, flags, 1, GETPC());                        \
> +    }                                                                         \

Existing bug, but this is missing to clear fp status to start.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> +                                                                              \
> +    return ret;                                                               \
>   }
>   
> -/* fsqrts - fsqrts. */
> -float64 helper_fsqrts(CPUPPCState *env, float64 arg)
> -{
> -    float64 ret = float64r32_sqrt(arg, &env->fp_status);
> -    int flags = get_float_exception_flags(&env->fp_status);
> -
> -    if (unlikely(flags & float_flag_invalid)) {
> -        float_invalid_op_sqrt(env, flags, 1, GETPC());
> -    }
> -    return ret;
> -}
> +FPU_FSQRT(FSQRT, float64_sqrt)
> +FPU_FSQRT(FSQRTS, float64r32_sqrt)
>   
>   /* fre - fre. */
>   float64 helper_fre(CPUPPCState *env, float64 arg)
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 159b352f6e..68610896b8 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64)
>   DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64)
>   DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64)
>   DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64)
> -DEF_HELPER_2(fsqrt, f64, env, f64)
> -DEF_HELPER_2(fsqrts, f64, env, f64)
> +DEF_HELPER_2(FSQRT, f64, env, f64)
> +DEF_HELPER_2(FSQRTS, f64, env, f64)
>   DEF_HELPER_2(fre, i64, env, i64)
>   DEF_HELPER_2(fres, i64, env, i64)
>   DEF_HELPER_2(frsqrte, i64, env, i64)
> diff --git a/target/ppc/translate/fp-impl.c.inc b/target/ppc/translate/fp-impl.c.inc
> index 7a90c0e350..8d5cf0f982 100644
> --- a/target/ppc/translate/fp-impl.c.inc
> +++ b/target/ppc/translate/fp-impl.c.inc
> @@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, arg_A_tb *a,
>       return true;
>   }
>   
> -TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);
> -TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts);
> +TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT);
> +TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS);
>   
>   /***                     Floating-Point multiply-and-add                   ***/
>   /* fmadd - fmadds */

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Víctor Colombo]

On 05/09/2022 12:56, Richard Henderson wrote:
> On 9/5/22 13:37, Víctor Colombo wrote:
>> These two helpers are almost identical, differing only by the softfloat
>> operation it calls. Merge them into one using a macro.
>> Also, take this opportunity to capitalize the helper name as we moved
>> the instruction to decodetree in a previous patch.
>>
>> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
>> ---
>>   target/ppc/fpu_helper.c            | 35 +++++++++++-------------------
>>   target/ppc/helper.h                |  4 ++--
>>   target/ppc/translate/fp-impl.c.inc |  4 ++--
>>   3 files changed, 17 insertions(+), 26 deletions(-)
>>
>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
>> index 0f045b70f8..32995179b5 100644
>> --- a/target/ppc/fpu_helper.c
>> +++ b/target/ppc/fpu_helper.c
>> @@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState 
>> *env, int flags,
>>       }
>>   }
>>
>> -/* fsqrt - fsqrt. */
>> -float64 helper_fsqrt(CPUPPCState *env, float64 arg)
>> -{
>> -    float64 ret = float64_sqrt(arg, &env->fp_status);
>> -    int flags = get_float_exception_flags(&env->fp_status);
>> -
>> -    if (unlikely(flags & float_flag_invalid)) {
>> -        float_invalid_op_sqrt(env, flags, 1, GETPC());
>> -    }
>> -
>> -    return ret;
>> +#define FPU_FSQRT(name, 
>> op)                                                   \
>> +float64 helper_##name(CPUPPCState *env, float64 
>> arg)                          \
>> +{                                                                             
>> \
>> +    float64 ret = op(arg, 
>> &env->fp_status);                                   \
>> +    int flags = 
>> get_float_exception_flags(&env->fp_status);                   \
>> +                                                                              
>> \
>> +    if (unlikely(flags & float_flag_invalid)) 
>> {                               \
>> +        float_invalid_op_sqrt(env, flags, 1, 
>> GETPC());                        \
>> +    
>> }                                                                         
>> \
> 
> Existing bug, but this is missing to clear fp status to start.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> r~
> 

Hello Richard, thanks for your review!
gen_reset_fpstatus() is called by the inline implementation in
do_helper_fsqrt() before calling the helper (patch 1).
It's probably better to move the call to inside the helper.

>> +                                                                              
>> \
>> +    return 
>> ret;                                                               \
>>   }
>>
>> -/* fsqrts - fsqrts. */
>> -float64 helper_fsqrts(CPUPPCState *env, float64 arg)
>> -{
>> -    float64 ret = float64r32_sqrt(arg, &env->fp_status);
>> -    int flags = get_float_exception_flags(&env->fp_status);
>> -
>> -    if (unlikely(flags & float_flag_invalid)) {
>> -        float_invalid_op_sqrt(env, flags, 1, GETPC());
>> -    }
>> -    return ret;
>> -}
>> +FPU_FSQRT(FSQRT, float64_sqrt)
>> +FPU_FSQRT(FSQRTS, float64r32_sqrt)
>>
>>   /* fre - fre. */
>>   float64 helper_fre(CPUPPCState *env, float64 arg)
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 159b352f6e..68610896b8 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64)
>>   DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64)
>>   DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64)
>>   DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64)
>> -DEF_HELPER_2(fsqrt, f64, env, f64)
>> -DEF_HELPER_2(fsqrts, f64, env, f64)
>> +DEF_HELPER_2(FSQRT, f64, env, f64)
>> +DEF_HELPER_2(FSQRTS, f64, env, f64)
>>   DEF_HELPER_2(fre, i64, env, i64)
>>   DEF_HELPER_2(fres, i64, env, i64)
>>   DEF_HELPER_2(frsqrte, i64, env, i64)
>> diff --git a/target/ppc/translate/fp-impl.c.inc 
>> b/target/ppc/translate/fp-impl.c.inc
>> index 7a90c0e350..8d5cf0f982 100644
>> --- a/target/ppc/translate/fp-impl.c.inc
>> +++ b/target/ppc/translate/fp-impl.c.inc
>> @@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, 
>> arg_A_tb *a,
>>       return true;
>>   }
>>
>> -TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);
>> -TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts);
>> +TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT);
>> +TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS);
>>
>>   /***                     Floating-Point 
>> multiply-and-add                   ***/
>>   /* fmadd - fmadds */
> 


-- 
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Richard Henderson]

On 9/5/22 17:19, Víctor Colombo wrote:
>> Existing bug, but this is missing to clear fp status to start.
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> r~
>>
> 
> Hello Richard, thanks for your review!
> gen_reset_fpstatus() is called by the inline implementation in
> do_helper_fsqrt() before calling the helper (patch 1).

Oops, ok.


> It's probably better to move the call to inside the helper.

I did write about a scheme by which all of these calls should go away.  I guess it has 
been a while...


r~

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Víctor Colombo]

On 05/09/2022 13:21, Richard Henderson wrote:
> On 9/5/22 17:19, Víctor Colombo wrote:
>>> Existing bug, but this is missing to clear fp status to start.
>>>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>
>>> r~
>>>
>>
>> Hello Richard, thanks for your review!
>> gen_reset_fpstatus() is called by the inline implementation in
>> do_helper_fsqrt() before calling the helper (patch 1).
> 
> Oops, ok.
> 
> 
>> It's probably better to move the call to inside the helper.
> 
> I did write about a scheme by which all of these calls should go away.  
> I guess it has
> been a while...
> 
> 
> r~

I have a message bookmarked here ([1]), but I don't know if there is a
previous one with a more in depth scheme.
Anyway, I was also analyzing recently the idea of removing all these
reset_fpstatus() calls from instructions helpers. I think this would
require to actually call it from the end of the (previous) instructions 
instead of the beginning? Like adding the call to
do_float_check_status() and float_invalid_op_*() as a focal point to
'hide' the calls to reset_fpstatus(). However there are also insns
helpers that don't call these auxiliary functions, which I think would
cause the refactor to not be worthy overall.
Did you have another idea that could be simpler?

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg00064.html


-- 
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Richard Henderson]

On 9/5/22 17:31, Víctor Colombo wrote:
> I have a message bookmarked here ([1]), but I don't know if there is a
> previous one with a more in depth scheme.

There may have been a previous, but that's the one I was thinking of.

> Anyway, I was also analyzing recently the idea of removing all these
> reset_fpstatus() calls from instructions helpers. I think this would
> require to actually call it from the end of the (previous) instructions instead of the 
> beginning? Like adding the call to
> do_float_check_status() and float_invalid_op_*() as a focal point to
> 'hide' the calls to reset_fpstatus().

Well, there would of course be no separate call, but do_float_check_status would:

     int status = get_float_exception_flags(&env->fp_status);

     set_float_exception_flags(0, &env->fp_status);

straight away.  No extra call overhead, and the steady-state of softfp exception flags 
outside of an in-progress fp operation is 0.

> However there are also insns
> helpers that don't call these auxiliary functions, which I think would
> cause the refactor to not be worthy overall.

Anything that can raise a softfp exception and doesn't do something with it, either 
immediately within the same helper, or just afterward with helper_float_check_status, is 
buggy.  With those fixed, helper_reset_fpstatus may be removed entirely.



r~

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Víctor Colombo]

On 05/09/2022 14:20, Richard Henderson wrote:
> Well, there would of course be no separate call, but 

I didn't understand what you meant here with 'no separate call'...

> do_float_check_status would:
> 
>      int status = get_float_exception_flags(&env->fp_status);
> 
>      set_float_exception_flags(0, &env->fp_status);
> 
> straight away.  No extra call overhead, and the steady-state of softfp 
> exception flags
> outside of an in-progress fp operation is 0.
> 

Right, makes sense. And what about when an invalid operation occurs,
with the corresponding exception enabled bit set?
float_invalid_op_* would stop the execution and do_float_check_status
would not be called, right? So it would require to call
set_float_exception_flags there too, correct?
If that's all that's necessary, I might be able to take a look at it and
come with a possible patch.

> Anything that can raise a softfp exception and doesn't do something with 
> it, either
> immediately within the same helper, or just afterward with 
> helper_float_check_status, is
> buggy.  With those fixed, helper_reset_fpstatus may be removed entirely.
> 

Oh, that makes sense. It's easier to implement the idea using this
assumption.


-- 
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH 0/3] target/ppc: Move fsqrt[s] to decodetree

[Daniel Henrique Barboza]

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

On 9/5/22 09:37, Víctor Colombo wrote:
> Move fsqrt and fsqrts instructions from decode legacy to decodetree.
> 
> Víctor Colombo (3):
>    target/ppc: Move fsqrt to decodetree
>    target/ppc: Move fsqrts to decodetree
>    target/ppc: Merge fsqrt and fsqrts helpers
> 
>   target/ppc/fpu_helper.c            | 35 ++++++++-------------
>   target/ppc/helper.h                |  4 +--
>   target/ppc/insn32.decode           |  8 +++++
>   target/ppc/translate/fp-impl.c.inc | 50 ++++++++++--------------------
>   target/ppc/translate/fp-ops.c.inc  |  2 --
>   5 files changed, 40 insertions(+), 59 deletions(-)
> 

Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

[Richard Henderson]

On 9/6/22 15:22, Víctor Colombo wrote:
> On 05/09/2022 14:20, Richard Henderson wrote:
>> Well, there would of course be no separate call, but 
> 
> I didn't understand what you meant here with 'no separate call'...

We generate a separate call from tcg to helper_reset_fpstatus sometimes.

> Right, makes sense. And what about when an invalid operation occurs,
> with the corresponding exception enabled bit set?
> float_invalid_op_* would stop the execution and do_float_check_status
> would not be called, right? So it would require to call
> set_float_exception_flags there too, correct?

Correct.


r~