diff mbox series

[v4,38/47] target/ppc: Refactor VSX_SCALAR_CMP_DP

Message ID 20220222143646.1268606-39-matheus.ferst@eldorado.org.br (mailing list archive)
State New, archived
Headers show
Series target/ppc: PowerISA Vector/VSX instruction batch | expand

Commit Message

Matheus K. Ferst Feb. 22, 2022, 2:36 p.m. UTC
From: Víctor Colombo <victor.colombo@eldorado.org.br>

Refactor VSX_SCALAR_CMP_DP, changing its name to VSX_SCALAR_CMP and
prepare the helper to be used for quadword comparisons.

Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/fpu_helper.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

Comments

Richard Henderson Feb. 23, 2022, 12:20 a.m. UTC | #1
On 2/22/22 04:36, matheus.ferst@eldorado.org.br wrote:
> From: Víctor Colombo <victor.colombo@eldorado.org.br>
> 
> Refactor VSX_SCALAR_CMP_DP, changing its name to VSX_SCALAR_CMP and
> prepare the helper to be used for quadword comparisons.
> 
> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>   target/ppc/fpu_helper.c | 31 ++++++++++++++-----------------
>   1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 9b034d1fe4..5ebbcfe3b7 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -2265,28 +2265,30 @@ VSX_MADDQ(XSNMSUBQP, NMSUB_FLGS, 0)
>   VSX_MADDQ(XSNMSUBQPO, NMSUB_FLGS, 0)
>   
>   /*
> - * VSX_SCALAR_CMP_DP - VSX scalar floating point compare double precision
> + * VSX_SCALAR_CMP - VSX scalar floating point compare
>    *   op    - instruction mnemonic
> + *   tp    - type
>    *   cmp   - comparison operation
>    *   exp   - expected result of comparison
> + *   fld   - vsr_t field
>    *   svxvc - set VXVC bit
>    */
> -#define VSX_SCALAR_CMP_DP(op, cmp, exp, svxvc)                                \
> +#define VSX_SCALAR_CMP(op, tp, cmp, fld, exp, svxvc)                          \
>   void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                             \
>                    ppc_vsr_t *xa, ppc_vsr_t *xb)                                \
>   {                                                                             \
> -    ppc_vsr_t t = *xt;                                                        \
> +    ppc_vsr_t t = { };                                                        \
>       bool vxsnan_flag = false, vxvc_flag = false, vex_flag = false;            \
>                                                                                 \
> -    if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) ||             \
> -        float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) {             \
> +    if (tp##_is_signaling_nan(xa->fld, &env->fp_status) ||                    \
> +        tp##_is_signaling_nan(xb->fld, &env->fp_status)) {                    \
>           vxsnan_flag = true;                                                   \
>           if (fpscr_ve == 0 && svxvc) {                                         \
>               vxvc_flag = true;                                                 \
>           }                                                                     \
>       } else if (svxvc) {                                                       \
> -        vxvc_flag = float64_is_quiet_nan(xa->VsrD(0), &env->fp_status) ||     \
> -            float64_is_quiet_nan(xb->VsrD(0), &env->fp_status);               \
> +        vxvc_flag = tp##_is_quiet_nan(xa->fld, &env->fp_status) ||            \
> +            tp##_is_quiet_nan(xb->fld, &env->fp_status);                      \
>       }                     

Note that this can be simplified further, using the full FloatRelation result and 
float_flag_invalid_snan.

Note that do_scalar_cmp gets half-way there, only checking for NaNs once we have 
float_relation_unordered as a comparision result.  But it could go further and check 
float_flag_invalid_snan and drop all of the other checks vs snan and qnan.


r~
Víctor Colombo Feb. 24, 2022, 7:16 p.m. UTC | #2
On 22/02/2022 21:20, Richard Henderson wrote:> On 2/22/22 04:36, 
matheus.ferst@eldorado.org.br wrote:
>> From: Víctor Colombo <victor.colombo@eldorado.org.br>
>>
>> Refactor VSX_SCALAR_CMP_DP, changing its name to VSX_SCALAR_CMP and
>> prepare the helper to be used for quadword comparisons.
>>
>> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>> ---
>>   target/ppc/fpu_helper.c | 31 ++++++++++++++-----------------
>>   1 file changed, 14 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
>> index 9b034d1fe4..5ebbcfe3b7 100644
>> --- a/target/ppc/fpu_helper.c
>> +++ b/target/ppc/fpu_helper.c
>> @@ -2265,28 +2265,30 @@ VSX_MADDQ(XSNMSUBQP, NMSUB_FLGS, 0)
>>   VSX_MADDQ(XSNMSUBQPO, NMSUB_FLGS, 0)
>>
>>   /*
>> - * VSX_SCALAR_CMP_DP - VSX scalar floating point compare double 
>> precision
>> + * VSX_SCALAR_CMP - VSX scalar floating point compare
>>    *   op    - instruction mnemonic
>> + *   tp    - type
>>    *   cmp   - comparison operation
>>    *   exp   - expected result of comparison
>> + *   fld   - vsr_t field
>>    *   svxvc - set VXVC bit
>>    */
>> -#define VSX_SCALAR_CMP_DP(op, cmp, exp, 
>> svxvc)                                \
>> +#define VSX_SCALAR_CMP(op, tp, cmp, fld, exp, 
>> svxvc)                          \
>>   void helper_##op(CPUPPCState *env, ppc_vsr_t 
>> *xt,                             \
>>                    ppc_vsr_t *xa, ppc_vsr_t 
>> *xb)                                \
>>   
>> {                                                                             
>> \
>> -    ppc_vsr_t t = 
>> *xt;                                                        \
>> +    ppc_vsr_t t = { 
>> };                                                        \
>>       bool vxsnan_flag = false, vxvc_flag = false, vex_flag = 
>> false;            \
>>                                                                                 
>> \
>> -    if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) 
>> ||             \
>> -        float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) 
>> {             \
>> +    if (tp##_is_signaling_nan(xa->fld, &env->fp_status) 
>> ||                    \
>> +        tp##_is_signaling_nan(xb->fld, &env->fp_status)) 
>> {                    \
>>           vxsnan_flag = 
>> true;                                                   \
>>           if (fpscr_ve == 0 && svxvc) 
>> {                                         \
>>               vxvc_flag = 
>> true;                                                 \
>>           
>> }                                                                     \
>>       } else if (svxvc) 
>> {                                                       \
>> -        vxvc_flag = float64_is_quiet_nan(xa->VsrD(0), 
>> &env->fp_status) ||     \
>> -            float64_is_quiet_nan(xb->VsrD(0), 
>> &env->fp_status);               \
>> +        vxvc_flag = tp##_is_quiet_nan(xa->fld, &env->fp_status) 
>> ||            \
>> +            tp##_is_quiet_nan(xb->fld, 
>> &env->fp_status);                      \
>>       }
> 
> Note that this can be simplified further, using the full FloatRelation 
> result and
> float_flag_invalid_snan.
> 
> Note that do_scalar_cmp gets half-way there, only checking for NaNs once 
> we have
> float_relation_unordered as a comparision result.  But it could go 
> further and check
> float_flag_invalid_snan and drop all of the other checks vs snan and qnan.
> 
> 
> r~

Hello Richard! Thanks for your review

Could you please elaborate more on how do you think using
float*_compare and its FloatRelation result would work here?
I noticed do_scalar_cmp modifies CR and sets FPCC flag, which
is not what VSX_SCALAR_CMP do. Using that function would require a
rework.

An option I though would be to bring into VSX_SCALAR_CMP the
important necessary parts, something like this:

#define VSX_SCALAR_CMP(op, tp, cmp, fld, svxvc, expr) 
       ...
     r = tp##_compare(xa->fld, xb->fld, &env->fp_status); 
        \
     if (expr) { 
        \
         memset(&t.fld, 0xFF, sizeof(t.fld)); 
        \
     } else if (r == float_relation_unordered) { 
        \
         if (env->fp_status.float_exception_flags & 
float_flag_invalid_snan) { \
             float_invalid_op_vxsnan(env, GETPC()); 
        \
             if (fpscr_ve == 0 && svxvc) { 
        \
                 float_invalid_op_vxvc(env, 0, GETPC()); 
        \
             } 
        \
         } else if (svxvc) { 
        \
             if (tp##_is_quiet_nan(xa->fld, &env->fp_status) || 
        \
                 tp##_is_quiet_nan(xb->fld, &env->fp_status)) { 
        \
                     float_invalid_op_vxvc(env, 0, GETPC()); 
        \
                 } 
        \
         } 
        \
     } 
        \
...
VSX_SCALAR_CMP(XSCMPEQDP, float64, eq, VsrD(0), 0, r == 
float_relation_equal)
VSX_SCALAR_CMP(XSCMPGEDP, float64, le, VsrD(0), 1, \
     r == float_relation_equal || r == float_relation_greater)
VSX_SCALAR_CMP(XSCMPGTDP, float64, lt, VsrD(0), 1, r == 
float_relation_greater)

But this still looks convoluted. Another option I came with would be:

     ppc_vsr_t t = { }; 
        \
 
        \
     helper_reset_fpstatus(env); 
        \
 
        \
     if (tp##_##cmp##_quiet(xb->fld, xa->fld, &env->fp_status)) { 
        \
         memset(&t.fld, 0xFF, sizeof(t.fld)); 
        \
     } 
        \
 
        \
     if (env->fp_status.float_exception_flags & float_flag_invalid_snan) 
{     \
         float_invalid_op_vxsnan(env, GETPC()); 
        \
         if (fpscr_ve == 0 && svxvc) { 
        \
             float_invalid_op_vxvc(env, 0, GETPC()); 
        \
         } 
        \
     } else if (svxvc) { 
        \
         if (tp##_is_quiet_nan(xa->fld, &env->fp_status) || 
        \
             tp##_is_quiet_nan(xb->fld, &env->fp_status)) { 
        \
                 float_invalid_op_vxvc(env, 0, GETPC()); 
        \
             } 
        \
     } 
        \

Is this close to what you were thinking?

Thank you very much!

-- Víctor
Richard Henderson Feb. 24, 2022, 9:24 p.m. UTC | #3
On 2/24/22 09:16, Víctor Colombo wrote:
> Could you please elaborate more on how do you think using
> float*_compare and its FloatRelation result would work here?
> I noticed do_scalar_cmp modifies CR and sets FPCC flag, which
> is not what VSX_SCALAR_CMP do. Using that function would require a
> rework.
> 
> An option I though would be to bring into VSX_SCALAR_CMP the
> important necessary parts, something like this:
> 
> #define VSX_SCALAR_CMP(op, tp, cmp, fld, svxvc, expr)       ...
>      r = tp##_compare(xa->fld, xb->fld, &env->fp_status);        \
>      if (expr) {        \
>          memset(&t.fld, 0xFF, sizeof(t.fld));        \
>      } else if (r == float_relation_unordered) {        \
>          if (env->fp_status.float_exception_flags & float_flag_invalid_snan) { \
>              float_invalid_op_vxsnan(env, GETPC());        \
>              if (fpscr_ve == 0 && svxvc) {        \
>                  float_invalid_op_vxvc(env, 0, GETPC());        \
>              }        \
>          } else if (svxvc) {        \
>              if (tp##_is_quiet_nan(xa->fld, &env->fp_status) ||        \
>                  tp##_is_quiet_nan(xb->fld, &env->fp_status)) {        \
>                      float_invalid_op_vxvc(env, 0, GETPC());        \
>                  }        \
>          }        \
>      }        \
> ...
> VSX_SCALAR_CMP(XSCMPEQDP, float64, eq, VsrD(0), 0, r == float_relation_equal)
> VSX_SCALAR_CMP(XSCMPGEDP, float64, le, VsrD(0), 1, \
>      r == float_relation_equal || r == float_relation_greater)
> VSX_SCALAR_CMP(XSCMPGTDP, float64, lt, VsrD(0), 1, r == float_relation_greater)

I was thinking along the lines of:

     bool r;
     int flags;

     helper_reset_fpstatus(env);
     if (svxvc) {
         r = tp##cmp(...);
     } else {
         r = tp##cmp##_quiet(...);
     }

     flags = get_float_exception_flags(&env->fp_status);
     if (unlikely(flags & float_flag_invalid)) {
         bool vxvc = svxvc;
         if (flags & float_flag_invalid_snan)) {
             float_invalid_op_vxsnan(...);
             vxvc &= fpscr_ve == 0;
         }
         if (vxvc) {
             float_invalid_op_vxvc(...);
         }
     }

     memset(xt, 0, sizeof(*xt));
     memset(&xt->fld, -r, sizeof(xt->fld));
     do_float_check_status(...);


r~
diff mbox series

Patch

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 9b034d1fe4..5ebbcfe3b7 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2265,28 +2265,30 @@  VSX_MADDQ(XSNMSUBQP, NMSUB_FLGS, 0)
 VSX_MADDQ(XSNMSUBQPO, NMSUB_FLGS, 0)
 
 /*
- * VSX_SCALAR_CMP_DP - VSX scalar floating point compare double precision
+ * VSX_SCALAR_CMP - VSX scalar floating point compare
  *   op    - instruction mnemonic
+ *   tp    - type
  *   cmp   - comparison operation
  *   exp   - expected result of comparison
+ *   fld   - vsr_t field
  *   svxvc - set VXVC bit
  */
-#define VSX_SCALAR_CMP_DP(op, cmp, exp, svxvc)                                \
+#define VSX_SCALAR_CMP(op, tp, cmp, fld, exp, svxvc)                          \
 void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                             \
                  ppc_vsr_t *xa, ppc_vsr_t *xb)                                \
 {                                                                             \
-    ppc_vsr_t t = *xt;                                                        \
+    ppc_vsr_t t = { };                                                        \
     bool vxsnan_flag = false, vxvc_flag = false, vex_flag = false;            \
                                                                               \
-    if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) ||             \
-        float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) {             \
+    if (tp##_is_signaling_nan(xa->fld, &env->fp_status) ||                    \
+        tp##_is_signaling_nan(xb->fld, &env->fp_status)) {                    \
         vxsnan_flag = true;                                                   \
         if (fpscr_ve == 0 && svxvc) {                                         \
             vxvc_flag = true;                                                 \
         }                                                                     \
     } else if (svxvc) {                                                       \
-        vxvc_flag = float64_is_quiet_nan(xa->VsrD(0), &env->fp_status) ||     \
-            float64_is_quiet_nan(xb->VsrD(0), &env->fp_status);               \
+        vxvc_flag = tp##_is_quiet_nan(xa->fld, &env->fp_status) ||            \
+            tp##_is_quiet_nan(xb->fld, &env->fp_status);                      \
     }                                                                         \
     if (vxsnan_flag) {                                                        \
         float_invalid_op_vxsnan(env, GETPC());                                \
@@ -2297,22 +2299,17 @@  void helper_##op(CPUPPCState *env, ppc_vsr_t *xt,                             \
     vex_flag = fpscr_ve && (vxvc_flag || vxsnan_flag);                        \
                                                                               \
     if (!vex_flag) {                                                          \
-        if (float64_##cmp(xb->VsrD(0), xa->VsrD(0),                           \
-                          &env->fp_status) == exp) {                          \
-            t.VsrD(0) = -1;                                                   \
-            t.VsrD(1) = 0;                                                    \
-        } else {                                                              \
-            t.VsrD(0) = 0;                                                    \
-            t.VsrD(1) = 0;                                                    \
+        if (tp##_##cmp(xb->fld, xa->fld, &env->fp_status) == exp) {           \
+            memset(&t.fld, 0xFF, sizeof(t.fld));                              \
         }                                                                     \
     }                                                                         \
     *xt = t;                                                                  \
     do_float_check_status(env, GETPC());                                      \
 }
 
-VSX_SCALAR_CMP_DP(xscmpeqdp, eq, 1, 0)
-VSX_SCALAR_CMP_DP(xscmpgedp, le, 1, 1)
-VSX_SCALAR_CMP_DP(xscmpgtdp, lt, 1, 1)
+VSX_SCALAR_CMP(xscmpeqdp, float64, eq, VsrD(0), 1, 0)
+VSX_SCALAR_CMP(xscmpgedp, float64, le, VsrD(0), 1, 1)
+VSX_SCALAR_CMP(xscmpgtdp, float64, lt, VsrD(0), 1, 1)
 
 void helper_xscmpexpdp(CPUPPCState *env, uint32_t opcode,
                        ppc_vsr_t *xa, ppc_vsr_t *xb)