[PATCH] ppc/translate: Fix unordered f64/f128 comparisons

LemonBoy posted 1 patch 3 years, 5 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201109102147.31747-1-thatlemon@gmail.com
Maintainers: David Gibson <david@gibson.dropbear.id.au>
There is a newer version of this series
target/ppc/fpu_helper.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
[PATCH] ppc/translate: Fix unordered f64/f128 comparisons
Posted by LemonBoy 3 years, 5 months ago
According to the PowerISA v3.1 reference, Table 68 "Actions for xscmpudp
- Part 1: Compare Unordered", whenever one of the two operands is a NaN
the SO bit is set while the other three bits are cleared.

Apply the same change to xscmpuqp.

The respective ordered counterparts are unaffected.

Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
---
 target/ppc/fpu_helper.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index 9b8c8b70b6..b07ff66375 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2479,13 +2479,11 @@ void helper_##op(CPUPPCState *env, uint32_t opcode,                      \
     if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) ||        \
         float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) {        \
         vxsnan_flag = true;                                              \
-        cc = CRF_SO;                                                     \
         if (fpscr_ve == 0 && ordered) {                                  \
             vxvc_flag = true;                                            \
         }                                                                \
     } else if (float64_is_quiet_nan(xa->VsrD(0), &env->fp_status) ||     \
                float64_is_quiet_nan(xb->VsrD(0), &env->fp_status)) {     \
-        cc = CRF_SO;                                                     \
         if (ordered) {                                                   \
             vxvc_flag = true;                                            \
         }                                                                \
@@ -2497,12 +2495,19 @@ void helper_##op(CPUPPCState *env, uint32_t opcode,                      \
         float_invalid_op_vxvc(env, 0, GETPC());                          \
     }                                                                    \
                                                                          \
-    if (float64_lt(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {         \
+    switch (float64_compare(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {\
+    case float_relation_less:                                            \
         cc |= CRF_LT;                                                    \
-    } else if (!float64_le(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \
-        cc |= CRF_GT;                                                    \
-    } else {                                                             \
+        break;                                                           \
+    case float_relation_equal:                                           \
         cc |= CRF_EQ;                                                    \
+        break;                                                           \
+    case float_relation_greater:                                         \
+        cc |= CRF_GT;                                                    \
+        break;                                                           \
+    case float_relation_unordered:                                       \
+        cc |= CRF_SO;                                                    \
+        break;                                                           \
     }                                                                    \
                                                                          \
     env->fpscr &= ~FP_FPCC;                                              \
@@ -2545,12 +2550,19 @@ void helper_##op(CPUPPCState *env, uint32_t opcode,                     \
         float_invalid_op_vxvc(env, 0, GETPC());                         \
     }                                                                   \
                                                                         \
-    if (float128_lt(xa->f128, xb->f128, &env->fp_status)) {             \
+    switch (float128_compare(xa->f128, xb->f128, &env->fp_status)) {    \
+    case float_relation_less:                                           \
         cc |= CRF_LT;                                                   \
-    } else if (!float128_le(xa->f128, xb->f128, &env->fp_status)) {     \
-        cc |= CRF_GT;                                                   \
-    } else {                                                            \
+        break;                                                          \
+    case float_relation_equal:                                          \
         cc |= CRF_EQ;                                                   \
+        break;                                                          \
+    case float_relation_greater:                                        \
+        cc |= CRF_GT;                                                   \
+        break;                                                          \
+    case float_relation_unordered:                                      \
+        cc |= CRF_SO;                                                   \
+        break;                                                          \
     }                                                                   \
                                                                         \
     env->fpscr &= ~FP_FPCC;                                             \
-- 
2.27.0


Re: [PATCH] ppc/translate: Fix unordered f64/f128 comparisons
Posted by Richard Henderson 3 years, 5 months ago
On 11/9/20 2:21 AM, LemonBoy wrote:
> According to the PowerISA v3.1 reference, Table 68 "Actions for xscmpudp
> - Part 1: Compare Unordered", whenever one of the two operands is a NaN
> the SO bit is set while the other three bits are cleared.
> 
> Apply the same change to xscmpuqp.
> 
> The respective ordered counterparts are unaffected.
> 
> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
> ---
>  target/ppc/fpu_helper.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index 9b8c8b70b6..b07ff66375 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -2479,13 +2479,11 @@ void helper_##op(CPUPPCState *env, uint32_t opcode,                      \
>      if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) ||        \
>          float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) {        \
>          vxsnan_flag = true;                                              \
> -        cc = CRF_SO;                                                     \
>          if (fpscr_ve == 0 && ordered) {                                  \
>              vxvc_flag = true;                                            \
>          }                                                                \
>      } else if (float64_is_quiet_nan(xa->VsrD(0), &env->fp_status) ||     \
>                 float64_is_quiet_nan(xb->VsrD(0), &env->fp_status)) {     \
> -        cc = CRF_SO;                                                     \
>          if (ordered) {                                                   \
>              vxvc_flag = true;                                            \
>          }                                                                \
> @@ -2497,12 +2495,19 @@ void helper_##op(CPUPPCState *env, uint32_t opcode,                      \
>          float_invalid_op_vxvc(env, 0, GETPC());                          \
>      }                                                                    \
>                                                                           \
> -    if (float64_lt(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {         \
> +    switch (float64_compare(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {\
> +    case float_relation_less:                                            \
>          cc |= CRF_LT;                                                    \
> -    } else if (!float64_le(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \
> -        cc |= CRF_GT;                                                    \
> -    } else {                                                             \
> +        break;                                                           \
> +    case float_relation_equal:                                           \
>          cc |= CRF_EQ;                                                    \
> +        break;                                                           \
> +    case float_relation_greater:                                         \
> +        cc |= CRF_GT;                                                    \
> +        break;                                                           \
> +    case float_relation_unordered:                                       \
> +        cc |= CRF_SO;                                                    \
> +        break;                                                           \
>      }                          

This needs some more cleanup.  There's no point in checking for nans first;
wait until you get to float_relation_unordered.

These macros should be made into straight functions.


r~

Re: [PATCH] ppc/translate: Fix unordered f64/f128 comparisons
Posted by LemonBoy 3 years, 5 months ago
On 10/11/20 04:24, Richard Henderson wrote:
> On 11/9/20 2:21 AM, LemonBoy wrote:
>> According to the PowerISA v3.1 reference, Table 68 "Actions for xscmpudp
>> - Part 1: Compare Unordered", whenever one of the two operands is a NaN
>> the SO bit is set while the other three bits are cleared.
>>
>> Apply the same change to xscmpuqp.
>>
>> The respective ordered counterparts are unaffected.
>>
>> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
>> ---
>>  target/ppc/fpu_helper.c | 32 ++++++++++++++++++++++----------
>>  1 file changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
>> index 9b8c8b70b6..b07ff66375 100644
>> --- a/target/ppc/fpu_helper.c
>> +++ b/target/ppc/fpu_helper.c
>> @@ -2479,13 +2479,11 @@ void helper_##op(CPUPPCState *env, uint32_t opcode,                      \
>>      if (float64_is_signaling_nan(xa->VsrD(0), &env->fp_status) ||        \
>>          float64_is_signaling_nan(xb->VsrD(0), &env->fp_status)) {        \
>>          vxsnan_flag = true;                                              \
>> -        cc = CRF_SO;                                                     \
>>          if (fpscr_ve == 0 && ordered) {                                  \
>>              vxvc_flag = true;                                            \
>>          }                                                                \
>>      } else if (float64_is_quiet_nan(xa->VsrD(0), &env->fp_status) ||     \
>>                 float64_is_quiet_nan(xb->VsrD(0), &env->fp_status)) {     \
>> -        cc = CRF_SO;                                                     \
>>          if (ordered) {                                                   \
>>              vxvc_flag = true;                                            \
>>          }                                                                \
>> @@ -2497,12 +2495,19 @@ void helper_##op(CPUPPCState *env, uint32_t opcode,                      \
>>          float_invalid_op_vxvc(env, 0, GETPC());                          \
>>      }                                                                    \
>>                                                                           \
>> -    if (float64_lt(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {         \
>> +    switch (float64_compare(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) {\
>> +    case float_relation_less:                                            \
>>          cc |= CRF_LT;                                                    \
>> -    } else if (!float64_le(xa->VsrD(0), xb->VsrD(0), &env->fp_status)) { \
>> -        cc |= CRF_GT;                                                    \
>> -    } else {                                                             \
>> +        break;                                                           \
>> +    case float_relation_equal:                                           \
>>          cc |= CRF_EQ;                                                    \
>> +        break;                                                           \
>> +    case float_relation_greater:                                         \
>> +        cc |= CRF_GT;                                                    \
>> +        break;                                                           \
>> +    case float_relation_unordered:                                       \
>> +        cc |= CRF_SO;                                                    \
>> +        break;                                                           \
>>      }                          
> 
> This needs some more cleanup.  There's no point in checking for nans first;
> wait until you get to float_relation_unordered.
> 
> These macros should be made into straight functions.
> 

Will do, I tried to keep the amount of changed lines as small as possible
but I'm happy to nuke that macro: aligning the trailing slashes took more
time than finding and fixing the bug :)

> 
> r~
>