Re: [PATCH] Remove alpha specific fmax, fmin to fix sNaN handling [BZ #20947]

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 31/12/2017 14:38, Aurelien Jarno wrote:
> Various fmax and fmin function implementations mishandle sNaN
> arguments:
> 
> (a) When both arguments are NaNs, the return value should be a qNaN,
> but sometimes it is an sNaN if at least one argument is an sNaN.
> 
> (b) Under TS 18661-1 semantics, if either argument is an sNaN then the
> result should be a qNaN (whereas if one argument is a qNaN and the
> other is not a NaN, the result should be the non-NaN argument).
> Various implementations treat sNaNs like qNaNs here.
> 
> One way to fix that is to detect the sNaN and add a special case. That
> said there is no FPU instruction to do that, so it requires transfering
> the FP value to an integer register and testing bits. This becomes quite
> complicated so it's probably better to just use the generic versions of
> these functions which just do that through issignaling.
> 
> Changelog:
> 	[BZ #20947]
> 	* sysdeps/alpha/fpu/s_fmax.S: Remove file.
> 	* sysdeps/alpha/fpu/s_fmaxf.S: Likewise.
> 	* sysdeps/alpha/fpu/s_fmin.S: Likewise.
> 	* sysdeps/alpha/fpu/s_fminf.S: Likewise.

LGTM. I think other alpha math functions suffers from similar issue (ceil and
floor at lest).

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  ChangeLog                   |  8 +++++++
>  sysdeps/alpha/fpu/s_fmax.S  | 52 ---------------------------------------------
>  sysdeps/alpha/fpu/s_fmaxf.S |  1 -
>  sysdeps/alpha/fpu/s_fmin.S  | 52 ---------------------------------------------
>  sysdeps/alpha/fpu/s_fminf.S |  1 -
>  5 files changed, 8 insertions(+), 106 deletions(-)
>  delete mode 100644 sysdeps/alpha/fpu/s_fmax.S
>  delete mode 100644 sysdeps/alpha/fpu/s_fmaxf.S
>  delete mode 100644 sysdeps/alpha/fpu/s_fmin.S
>  delete mode 100644 sysdeps/alpha/fpu/s_fminf.S
> 
> diff --git a/ChangeLog b/ChangeLog
> index cd6fc15767..3f6002a175 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,11 @@
> +2017-12-31  Aurelien Jarno  <aurelien@aurel32.net>
> +
> +	[BZ #20947]
> +	* sysdeps/alpha/fpu/s_fmax.S: Remove file.
> +	* sysdeps/alpha/fpu/s_fmaxf.S: Likewise.
> +	* sysdeps/alpha/fpu/s_fmin.S: Likewise.
> +	* sysdeps/alpha/fpu/s_fminf.S: Likewise.
> +
>  2017-12-30  Aurelien Jarno  <aurelien@aurel32.net>
>  	    Dmitry V. Levin  <ldv@altlinux.org>
>  
> diff --git a/sysdeps/alpha/fpu/s_fmax.S b/sysdeps/alpha/fpu/s_fmax.S
> deleted file mode 100644
> index 5da9e0df11..0000000000
> --- a/sysdeps/alpha/fpu/s_fmax.S
> +++ /dev/null
> @@ -1,52 +0,0 @@
> -/* Copyright (C) 2007-2017 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -   Contributed by Richard Henderson.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library.  If not, see
> -   <http://www.gnu.org/licenses/>.  */
> -
> -#include <sysdep.h>
> -#include <math_ldbl_opt.h>
> -#include <libm-alias-float.h>
> -#include <libm-alias-double.h>
> -
> -        .set noat
> -	.set noreorder
> -
> -	.text
> -ENTRY (__fmax)
> -	.prologue 0
> -
> -	cmptun/su	$f16, $f16, $f10
> -	cmptun/su	$f17, $f17, $f11
> -	fmov		$f17, $f0
> -	unop
> -
> -	trapb
> -	fbne		$f10, $ret
> -	fmov		$f16, $f0
> -	fbne		$f11, $ret
> -
> -	cmptlt/su	$f16, $f17, $f11
> -	trapb
> -	fcmovne		$f11, $f17, $f0
> -$ret:	ret
> -
> -END (__fmax)
> -
> -/* Given the in-register format of single-precision, this works there too.  */
> -strong_alias (__fmax, __fmaxf)
> -libm_alias_float (__fmax, fmax)
> -
> -libm_alias_double (__fmax, fmax)
> diff --git a/sysdeps/alpha/fpu/s_fmaxf.S b/sysdeps/alpha/fpu/s_fmaxf.S
> deleted file mode 100644
> index 3c2d62bb81..0000000000
> --- a/sysdeps/alpha/fpu/s_fmaxf.S
> +++ /dev/null
> @@ -1 +0,0 @@
> -/* __fmaxf is in s_fmax.c  */
> diff --git a/sysdeps/alpha/fpu/s_fmin.S b/sysdeps/alpha/fpu/s_fmin.S
> deleted file mode 100644
> index d752223151..0000000000
> --- a/sysdeps/alpha/fpu/s_fmin.S
> +++ /dev/null
> @@ -1,52 +0,0 @@
> -/* Copyright (C) 2007-2017 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -   Contributed by Richard Henderson.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library.  If not, see
> -   <http://www.gnu.org/licenses/>.  */
> -
> -#include <sysdep.h>
> -#include <math_ldbl_opt.h>
> -#include <libm-alias-float.h>
> -#include <libm-alias-double.h>
> -
> -        .set noat
> -	.set noreorder
> -
> -	.text
> -ENTRY (__fmin)
> -	.prologue 0
> -
> -	cmptun/su	$f16, $f16, $f10
> -	cmptun/su	$f17, $f17, $f11
> -	fmov		$f17, $f0
> -	unop
> -
> -	trapb
> -	fbne		$f10, $ret
> -	fmov		$f16, $f0
> -	fbne		$f11, $ret
> -
> -	cmptlt/su	$f17, $f16, $f11
> -	trapb
> -	fcmovne		$f11, $f17, $f0
> -$ret:	ret
> -
> -END (__fmin)
> -
> -/* Given the in-register format of single-precision, this works there too.  */
> -strong_alias (__fmin, __fminf)
> -libm_alias_float (__fmin, fmin)
> -
> -libm_alias_double (__fmin, fmin)
> diff --git a/sysdeps/alpha/fpu/s_fminf.S b/sysdeps/alpha/fpu/s_fminf.S
> deleted file mode 100644
> index 10ab7fe53c..0000000000
> --- a/sysdeps/alpha/fpu/s_fminf.S
> +++ /dev/null
> @@ -1 +0,0 @@
> -/* __fminf is in s_fmin.c  */
>