Re: [Qemu-devel] [PATCH v2 07/20] fpu/softfloat: propagate signalling NaNs in MINMAX

["Alex Bennée" <alex.bennee@linaro.org>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 January 2018 at 12:22, Alex Bennée <alex.bennee@linaro.org> wrote:
>> While a comparison between a QNaN and a number will return the number
>> it is not the same with a signaling NaN. In this case the SNaN will
>> "win" and after potentially raising an exception it will be quietened.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> v2
>>   - added return for propageFloat
>> ---
>>  fpu/softfloat.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
>> index 3a4ab1355f..44c043924e 100644
>> --- a/fpu/softfloat.c
>> +++ b/fpu/softfloat.c
>> @@ -7683,6 +7683,7 @@ int float128_compare_quiet(float128 a, float128 b, float_status *status)
>>   * minnum() and maxnum() functions. These are similar to the min()
>>   * and max() functions but if one of the arguments is a QNaN and
>>   * the other is numerical then the numerical argument is returned.
>> + * SNaNs will get quietened before being returned.
>>   * minnum() and maxnum correspond to the IEEE 754-2008 minNum()
>>   * and maxNum() operations. min() and max() are the typical min/max
>>   * semantics provided by many CPUs which predate that specification.
>> @@ -7703,11 +7704,14 @@ static inline float ## s float ## s ## _minmax(float ## s a, float ## s b,     \
>>      if (float ## s ## _is_any_nan(a) ||                                 \
>>          float ## s ## _is_any_nan(b)) {                                 \
>>          if (isieee) {                                                   \
>> -            if (float ## s ## _is_quiet_nan(a, status) &&               \
>> +            if (float ## s ## _is_signaling_nan(a, status) ||           \
>> +                float ## s ## _is_signaling_nan(b, status)) {           \
>> +                return propagateFloat ## s ## NaN(a, b, status);        \
>> +            } else  if (float ## s ## _is_quiet_nan(a, status) &&       \
>>                  !float ## s ##_is_any_nan(b)) {                         \
>>                  return b;                                               \
>>              } else if (float ## s ## _is_quiet_nan(b, status) &&        \
>> -                       !float ## s ## _is_any_nan(a)) {                \
>> +                       !float ## s ## _is_any_nan(a)) {                 \
>>                  return a;                                               \
>>              }                                                           \
>>          }                                                               \
>>          return propagateFloat ## s ## NaN(a, b, status);                \
>>      }                                                                   \
>
> [added a couple of extra lines of context at the end for clarity]
>
> Am I misreading this patch? I can't see in what case it makes a
> difference to the result. The code change adds an explicit "if
> either A or B is an SNaN then return the propagateFloat*NaN() result".
> But if either A or B is an SNaN then we won't take either of the
> previously existing branches in this if() ("if A is a QNaN and B is
> not a NaN" and "if B is a QNaN and A is not a NaN"), and so we'll
> end up falling through to the "return propagateFloat*NaN" line after
> the end of the "is (ieee) {...}".

I see your point. However the bug is there if we don't check for
signalling NaNs first which probably means the xxx_is_quiet_nan() check
is broken and reporting signalling NaN's as quiet.

The logic is correct in the decomposed function as we have many NaN
types to check against so we check for the signalling NaNs first.

>
> thanks
> -- PMM


--
Alex Bennée