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

[LemonBoy]

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~
>