[gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[FX Coudert]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

Attached patch makes MIN and MAX intrinsics return correct IEEE  
results (and consistent results) when given one or more NaN as  
arguments. It does so by changing the code generated from MAX(a,b) :=  
(b > a ? b : a) into MAX(a,b) := (b > a || isnan(a) ? b : a);  
handling of multiple arguments is done similarly.

The introduction of isnan() tests probably has a cost, but hey, we  
want a correct result anyway, don't we? Thus, I added a note into the  
code saying that when IEEE_ARITHMETIC is implemented, we might want  
to make this codepath dependent on IEEE_ARITHMETIC being used (I  
don't think we should do that, but we should certainly discuss the  
option).

I also added a testcase, I don't have much experience with NaNs so I  
don't know if all targets have them, does someone know if "x = 0.0 ;  
x = x / x;" will result in a NaN in all cases?

Bootstrapped and regtested on x86_64-linux, OK for mainline?
FX

:ADDPATH fortran:

[-- Attachment #2: minmax_nan.ChangeLog --]
[-- Type: application/octet-stream, Size: 344 bytes --]

2007-07-28  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

	PR fortran/32048
	* f95-lang.c (gfc_init_builtin_functions): Add declaration for
	__builtin_isnan.
	* trans-intrinsic.c (gfc_conv_intrinsic_minmax): Handled NaNs.


2007-07-28  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

	PR fortran/32048
	* gfortran.dg/nan_1.f90: New test.

[-- Attachment #3: minmax_nan.diff --]
[-- Type: application/octet-stream, Size: 6839 bytes --]

Index: gcc/fortran/f95-lang.c
===================================================================
--- gcc/fortran/f95-lang.c	(revision 126997)
+++ gcc/fortran/f95-lang.c	(working copy)
@@ -1004,6 +1004,11 @@ gfc_init_builtin_functions (void)
 		      "malloc", false);
   DECL_IS_MALLOC (built_in_decls[BUILT_IN_MALLOC]) = 1;
 
+  tmp = tree_cons (NULL_TREE, void_type_node, void_list_node);
+  ftype = build_function_type (integer_type_node, tmp);
+  gfc_define_builtin ("__builtin_isnan", ftype, BUILT_IN_ISNAN,
+		      "__builtin_isnan", true);
+
 #define DEF_PRIMITIVE_TYPE(ENUM, VALUE) \
   builtin_types[(int) ENUM] = VALUE;
 #define DEF_FUNCTION_TYPE_0(ENUM, RETURN)		\
Index: gcc/fortran/trans-intrinsic.c
===================================================================
--- gcc/fortran/trans-intrinsic.c	(revision 126997)
+++ gcc/fortran/trans-intrinsic.c	(working copy)
@@ -1407,11 +1407,11 @@ gfc_conv_intrinsic_ttynam (gfc_se * se, 
 /* Get the minimum/maximum value of all the parameters.
     minmax (a1, a2, a3, ...)
     {
-      if (a2 .op. a1)
+      if (a2 .op. a1 || isnan(a1))
         mvar = a2;
       else
         mvar = a1;
-      if (a3 .op. mvar)
+      if (a3 .op. mvar || isnan(mvar))
         mvar = a3;
       ...
       return mvar
@@ -1487,7 +1487,7 @@ gfc_conv_intrinsic_minmax (gfc_se * se, 
   elsecase = build2_v (MODIFY_EXPR, mvar, limit);
   for (i = 1, argexpr = argexpr->next; i < nargs; i++)
     {
-      tree cond;
+      tree cond, isnan;
 
       val = args[i]; 
 
@@ -1509,6 +1509,15 @@ gfc_conv_intrinsic_minmax (gfc_se * se, 
       thencase = build2_v (MODIFY_EXPR, mvar, convert (type, val));
 
       tmp = build2 (op, boolean_type_node, convert (type, val), limit);
+
+      /* FIXME: When the IEEE_ARITHMETIC module is implemented, the call to
+	 __builtin_isnan might be made dependent on that module being loaded,
+	 to help performance of programs that don't rely on IEEE semantics.  */
+      if (FLOAT_TYPE_P (TREE_TYPE (limit)))
+	{
+	  isnan = build_call_expr (built_in_decls[BUILT_IN_ISNAN], 1, limit);
+	  tmp = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, tmp, isnan);
+	}
       tmp = build3_v (COND_EXPR, tmp, thencase, elsecase);
 
       if (cond != NULL_TREE)
Index: gcc/testsuite/gfortran.dg/nan_1.f90
===================================================================
--- gcc/testsuite/gfortran.dg/nan_1.f90	(revision 0)
+++ gcc/testsuite/gfortran.dg/nan_1.f90	(revision 0)
@@ -0,0 +1,124 @@
+! Test if MIN and MAX intrinsics behave correctly when passed NaNs
+! as arguments
+!
+! { dg-do run }
+!
+module aux
+  interface isnan
+    module procedure isnan_r
+    module procedure isnan_d
+  end interface isnan
+
+  interface isinf
+    module procedure isinf_r
+    module procedure isinf_d
+  end interface isinf
+contains
+
+  pure function isnan_r(x) result (isnan)
+    logical :: isnan
+    real, intent(in) :: x
+
+    isnan = (.not.(x == x))
+  end function isnan_r
+
+  pure function isnan_d(x) result (isnan)
+    logical :: isnan
+    double precision, intent(in) :: x
+
+    isnan = (.not.(x == x))
+  end function isnan_d
+
+  pure function isinf_r(x) result (isinf)
+    logical :: isinf
+    real, intent(in) :: x
+
+    isinf = (x > huge(x)) .or. (x < -huge(x))
+  end function isinf_r
+
+  pure function isinf_d(x) result (isinf)
+    logical :: isinf
+    double precision, intent(in) :: x
+
+    isinf = (x > huge(x)) .or. (x < -huge(x))
+  end function isinf_d
+end module aux
+
+program test
+  use aux
+  implicit none
+  real :: nan, large, inf
+
+  ! Create a NaN and check it
+  nan = 0
+  nan = nan / nan
+  if (nan == nan .or. nan > nan .or. nan < nan .or. nan >= nan &
+      .or. nan <= nan) call abort
+  if (isnan (2.d0) .or. (.not. isnan(nan)) .or. &
+      (.not. isnan(real(nan,kind=kind(2.d0))))) call abort
+
+  ! Create an INF and check it
+  large = huge(large)
+  inf = 2 * large
+  if (isinf(nan) .or. isinf(large) .or. .not. isinf(inf)) call abort
+  if (isinf(-nan) .or. isinf(-large) .or. .not. isinf(-inf)) call abort
+
+  ! Check that MIN and MAX behave correctly
+  if (max(2.0, nan) /= 2.0) call abort
+  if (min(2.0, nan) /= 2.0) call abort
+  if (max(nan, 2.0) /= 2.0) call abort
+  if (min(nan, 2.0) /= 2.0) call abort
+
+  if (max(2.d0, nan) /= 2.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+  if (min(2.d0, nan) /= 2.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+  if (max(nan, 2.d0) /= 2.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+  if (min(nan, 2.d0) /= 2.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+
+  if (.not. isnan(min(nan,nan))) call abort
+  if (.not. isnan(max(nan,nan))) call abort
+
+  ! Same thing, with more arguments
+
+  if (max(3.0, 2.0, nan) /= 3.0) call abort
+  if (min(3.0, 2.0, nan) /= 2.0) call abort
+  if (max(3.0, nan, 2.0) /= 3.0) call abort
+  if (min(3.0, nan, 2.0) /= 2.0) call abort
+  if (max(nan, 3.0, 2.0) /= 3.0) call abort
+  if (min(nan, 3.0, 2.0) /= 2.0) call abort
+
+  if (max(3.d0, 2.d0, nan) /= 3.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+  if (min(3.d0, 2.d0, nan) /= 2.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+  if (max(3.d0, nan, 2.d0) /= 3.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+  if (min(3.d0, nan, 2.d0) /= 2.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+  if (max(nan, 3.d0, 2.d0) /= 3.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+  if (min(nan, 3.d0, 2.d0) /= 2.d0) call abort ! { dg-warning "Extension: Different type kinds" }
+
+  if (.not. isnan(min(nan,nan,nan))) call abort
+  if (.not. isnan(max(nan,nan,nan))) call abort
+  if (.not. isnan(min(nan,nan,nan,nan))) call abort
+  if (.not. isnan(max(nan,nan,nan,nan))) call abort
+  if (.not. isnan(min(nan,nan,nan,nan,nan))) call abort
+  if (.not. isnan(max(nan,nan,nan,nan,nan))) call abort
+
+  ! Large values, INF and NaNs
+  if (.not. isinf(max(large, inf))) call abort
+  if (isinf(min(large, inf))) call abort
+  if (.not. isinf(max(nan, large, inf))) call abort
+  if (isinf(min(nan, large, inf))) call abort
+  if (.not. isinf(max(large, nan, inf))) call abort
+  if (isinf(min(large, nan, inf))) call abort
+  if (.not. isinf(max(large, inf, nan))) call abort
+  if (isinf(min(large, inf, nan))) call abort
+
+  if (.not. isinf(min(-large, -inf))) call abort
+  if (isinf(max(-large, -inf))) call abort
+  if (.not. isinf(min(nan, -large, -inf))) call abort
+  if (isinf(max(nan, -large, -inf))) call abort
+  if (.not. isinf(min(-large, nan, -inf))) call abort
+  if (isinf(max(-large, nan, -inf))) call abort
+  if (.not. isinf(min(-large, -inf, nan))) call abort
+  if (isinf(max(-large, -inf, nan))) call abort
+
+end program test
+
+! { dg-final { cleanup-modules "aux" } }

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[Jerry DeLisle]

FX Coudert wrote:
> Attached patch makes MIN and MAX intrinsics return correct IEEE results 
> (and consistent results) when given one or more NaN as arguments. It 
> does so by changing the code generated from MAX(a,b) := (b > a ? b : a) 
> into MAX(a,b) := (b > a || isnan(a) ? b : a); handling of multiple 
> arguments is done similarly.
> 
> The introduction of isnan() tests probably has a cost, but hey, we want 
> a correct result anyway, don't we? Thus, I added a note into the code 
> saying that when IEEE_ARITHMETIC is implemented, we might want to make 
> this codepath dependent on IEEE_ARITHMETIC being used (I don't think we 
> should do that, but we should certainly discuss the option).
> 
> I also added a testcase, I don't have much experience with NaNs so I 
> don't know if all targets have them, does someone know if "x = 0.0 ; x = 
> x / x;" will result in a NaN in all cases?

Take a look in the testsuite at real_constant_3.f90.  I suppose this is really 
only testing mpfr.  I wonder if we should modify this or come up with a new test 
case that forces these values to be computed at run time.
> 
> Bootstrapped and regtested on x86_64-linux, OK for mainline?
> FX
> 
> :ADDPATH fortran:
This is OK.

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[Tim Prince]

FX Coudert wrote:
> Attached patch makes MIN and MAX intrinsics return correct IEEE 
> results (and consistent results) when given one or more NaN as 
> arguments. It does so by changing the code generated from MAX(a,b) := 
> (b > a ? b : a) into MAX(a,b) := (b > a || isnan(a) ? b : a); handling 
> of multiple arguments is done similarly.
>
> The introduction of isnan() tests probably has a cost, but hey, we 
> want a correct result anyway, don't we? Thus, I added a note into the 
> code saying that when IEEE_ARITHMETIC is implemented, we might want to 
> make this codepath dependent on IEEE_ARITHMETIC being used (I don't 
> think we should do that, but we should certainly discuss the option).
>
Does this affect minval() and maxval() intrinsics?  Up to now, they 
performed as well as equivalent C code with icpc, better than g++, but 
not as well as ifort.  I suppose it removes the prospect of vectorizing 
code with min() and max(), unless that will be enabled with -ffast-math.

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[Andrew Pinski]

On 7/28/07, Tim Prince <tprince@myrealbox.com> wrote:
> FX Coudert wrote:
> Does this affect minval() and maxval() intrinsics?  Up to now, they
> performed as well as equivalent C code with icpc, better than g++, but
> not as well as ifort.  I suppose it removes the prospect of vectorizing
> code with min() and max(), unless that will be enabled with -ffast-math.

Actually the G++ code should be better on the trunk than before.  If
it is not, can you file a new bug?

Thanks,
Andrew Pinski

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[Tim Prince]

pinskia@gmail.com wrote:
> On 7/28/07, Tim Prince <tprince@myrealbox.com> wrote:
>> FX Coudert wrote:
>> Does this affect minval() and maxval() intrinsics?  Up to now, they
>> performed as well as equivalent C code with icpc, better than g++, but
>> not as well as ifort.  I suppose it removes the prospect of vectorizing
>> code with min() and max(), unless that will be enabled with -ffast-math.
> 
> Actually the G++ code should be better on the trunk than before.  If
> it is not, can you file a new bug?
> 

I did find several cases where g++ x86-64 performance improved greatly 
in the last month, but this is not one of them.
I didn't find anything clear to complain about.  g++ is generating a lot 
of useless (on Core 2) p2align, but that does not correlate with 
performance.  g++ uses minps memory to register operations, while 
gfortran loads each operand first.  This looks like a way for g++ to 
favor reduced code size over performance (somewhat contradictory, after 
consuming space with p2align).  It might be better to unroll less and 
pre-load the operands.  I didn't think this is worth a PR, when I have 
plenty of long-standing PRs.

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[FX Coudert]

> Does this affect minval() and maxval() intrinsics?

No.

FX

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[Paul Thomas]

FX,

The testsuite now hangs in nan_1.f90 on Cygwin_NT/amd_64.  I have never 
seen this happen before - it does not even time out!  It hangs in 
compilation - if you have any thoughts on how this might be happening, 
let me know and I'll take a look-see.

Paul

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[FX Coudert]

> The testsuite now hangs in nan_1.f90 on Cygwin_NT/amd_64.  I have  
> never seen this happen before - it does not even time out!  It  
> hangs in compilation - if you have any thoughts on how this might  
> be happening, let me know and I'll take a look-see.

Yuck :(

No idea right now, and I don't have cygwin fully setup yet; if you  
run f951 under gdb, and manually break, where is it stuck?

FX

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[François-Xavier Coudert]

>> The testsuite now hangs in nan_1.f90 on Cygwin_NT/amd_64.  I have
>> never seen this happen before - it does not even time out!  It
>> hangs in compilation - if you have any thoughts on how this might
>> be happening, let me know and I'll take a look-see.
>
> Yuck :(
>
> No idea right now, and I don't have cygwin fully setup yet; if you
> run f951 under gdb, and manually break, where is it stuck?

OK, I've built a cygwin compiler and now I see where it hangs... it
has nothing to do with my patch, a reduced testcase is:

    module aux
    end module aux

Apparently, the compiler then hangs in KERNEL32!GetConsoleCharType. I
think this has something to do with AUX being a special device
somewhere in Windows (a google search for "cygwin aux" brings back
lots of stuff).

So, to avoid this snag, I've changed the module name in the testcase
into aux2 (committed as rev. 127240). The testcase now passes.

FX

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[Andrew Pinski]

On 8/6/07, François-Xavier Coudert <fxcoudert@gmail.com> wrote:
> Apparently, the compiler then hangs in KERNEL32!GetConsoleCharType. I
> think this has something to do with AUX being a special device
> somewhere in Windows (a google search for "cygwin aux" brings back
> lots of stuff).


Yes aux is the Auxiliary device driver.  You cannot use this name even
with a file extension.  So it might be a good idea to change the
testsuite.

Thanks,
Andrew Pinski

Re: [gfortran,patch] Make MIN and MAX handle NaNs correctly (PR32048)

[FX Coudert]

> The testsuite now hangs in nan_1.f90 on Cygwin_NT/amd_64.  I have  
> never seen this happen before - it does not even time out!  It  
> hangs in compilation - if you have any thoughts on how this might  
> be happening, let me know and I'll take a look-see.

Any news? Anyone else sees this?

FX