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

[Peter Maydell]

On 9 January 2018 at 12:22, Alex Bennée <address@hidden> 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 <address@hidden>
> Reviewed-by: Richard Henderson <address@hidden>
> ---
> 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) {...}".

thanks
-- PMM