match.pd: Don't fold nan < x etc. for -ftrapping-math [PR106805]

Message ID Y43H+LkdiDQjILIU@tucnak
State New
Headers
Series match.pd: Don't fold nan < x etc. for -ftrapping-math [PR106805] |

Commit Message

Jakub Jelinek Dec. 5, 2022, 10:29 a.m. UTC
  Hi!

As reported in the PR, the following pr106805.c testcase is miscompiled
with the default -ftrapping-math, because we fold all the comparisons into
constants and don't raise any exceptions.

The match.pd pattern handles just simple comparisons, from those
EQ/NE are quiet and don't raise exceptions on anything but sNaN, while
GT/GE/LT/LE are signaling and do raise exceptions even on qNaN.

fold_relational_const handles this IMHO correctly:
      /* Handle the cases where either operand is a NaN.  */
      if (real_isnan (c0) || real_isnan (c1))
        {
          switch (code)
            {
            case EQ_EXPR:
            case ORDERED_EXPR:
              result = 0;
              break;
       
            case NE_EXPR:
            case UNORDERED_EXPR:
            case UNLT_EXPR:
            case UNLE_EXPR:
            case UNGT_EXPR:
            case UNGE_EXPR:
            case UNEQ_EXPR:
              result = 1;
              break;
        
            case LT_EXPR:
            case LE_EXPR:
            case GT_EXPR:
            case GE_EXPR:
            case LTGT_EXPR:
              if (flag_trapping_math)
                return NULL_TREE;
              result = 0;
              break;

            default:
              gcc_unreachable ();
            }
            
          return constant_boolean_node (result, type);
        }
by folding the signaling comparisons only if -fno-trapping-math.
The following patch does the same in match.pd.

Unfortunately the pr106805.c testcase still fails, but no longer because of
match.pd, but on the trunk because of the still unresolved ranger problems
(same issue as for fold-overflow-1.c etc.) and on 12 branch (and presumably
trunk too) somewhere during expansion the comparisons are also expanded
into constants (which is ok for -fno-trapping-math, but not ok with that).

Though, I think the patch is a small step in the direction, so I'd like
to commit this patch without the gcc.dg/pr106805.c testcase for now.

Bootstrapped/regtested on x86_64-linux and i686-linux, appart from the
+FAIL: gcc.dg/pr106805.c execution test
nothing else appears.  Ok for trunk without the last testcase?

2022-12-05  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/106805
	* match.pd (cmp @0 REAL_CST@1): Don't optimize x cmp NaN
	or NaN cmp x to false/true for cmp >/>=/</<= if -ftrapping-math.

	* c-c++-common/pr57371-4.c: Revert 2021-09-19 changes.
	* c-c++-common/pr57371-5.c: New test.
	* gcc.c-torture/execute/ieee/fp-cmp-6.x: Add -fno-trapping-math.
	* gcc.c-torture/execute/ieee/fp-cmp-9.c: New test. 
	* gcc.c-torture/execute/ieee/fp-cmp-9.x: New file.
	* gcc.dg/pr106805.c: New test.


	Jakub
  

Comments

Richard Biener Dec. 5, 2022, 10:50 a.m. UTC | #1
> Am 05.12.2022 um 11:30 schrieb Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> Hi!
> 
> As reported in the PR, the following pr106805.c testcase is miscompiled
> with the default -ftrapping-math, because we fold all the comparisons into
> constants and don't raise any exceptions.
> 
> The match.pd pattern handles just simple comparisons, from those
> EQ/NE are quiet and don't raise exceptions on anything but sNaN, while
> GT/GE/LT/LE are signaling and do raise exceptions even on qNaN.
> 
> fold_relational_const handles this IMHO correctly:
>      /* Handle the cases where either operand is a NaN.  */
>      if (real_isnan (c0) || real_isnan (c1))
>        {
>          switch (code)
>            {
>            case EQ_EXPR:
>            case ORDERED_EXPR:
>              result = 0;
>              break;
> 
>            case NE_EXPR:
>            case UNORDERED_EXPR:
>            case UNLT_EXPR:
>            case UNLE_EXPR:
>            case UNGT_EXPR:
>            case UNGE_EXPR:
>            case UNEQ_EXPR:
>              result = 1;
>              break;
> 
>            case LT_EXPR:
>            case LE_EXPR:
>            case GT_EXPR:
>            case GE_EXPR:
>            case LTGT_EXPR:
>              if (flag_trapping_math)
>                return NULL_TREE;
>              result = 0;
>              break;
> 
>            default:
>              gcc_unreachable ();
>            }
> 
>          return constant_boolean_node (result, type);
>        }
> by folding the signaling comparisons only if -fno-trapping-math.
> The following patch does the same in match.pd.
> 
> Unfortunately the pr106805.c testcase still fails, but no longer because of
> match.pd, but on the trunk because of the still unresolved ranger problems
> (same issue as for fold-overflow-1.c etc.) and on 12 branch (and presumably
> trunk too) somewhere during expansion the comparisons are also expanded
> into constants (which is ok for -fno-trapping-math, but not ok with that).
> 
> Though, I think the patch is a small step in the direction, so I'd like
> to commit this patch without the gcc.dg/pr106805.c testcase for now.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, appart from the
> +FAIL: gcc.dg/pr106805.c execution test
> nothing else appears.  Ok for trunk without the last testcase?

Ok.

Richard 

> 2022-12-05  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR middle-end/106805
>    * match.pd (cmp @0 REAL_CST@1): Don't optimize x cmp NaN
>    or NaN cmp x to false/true for cmp >/>=/</<= if -ftrapping-math.
> 
>    * c-c++-common/pr57371-4.c: Revert 2021-09-19 changes.
>    * c-c++-common/pr57371-5.c: New test.
>    * gcc.c-torture/execute/ieee/fp-cmp-6.x: Add -fno-trapping-math.
>    * gcc.c-torture/execute/ieee/fp-cmp-9.c: New test. 
>    * gcc.c-torture/execute/ieee/fp-cmp-9.x: New file.
>    * gcc.dg/pr106805.c: New test.
> 
> --- gcc/match.pd.jj    2022-12-02 10:28:30.000000000 +0100
> +++ gcc/match.pd    2022-12-02 12:50:28.701735269 +0100
> @@ -5146,12 +5146,14 @@ (define_operator_list SYNC_FETCH_AND_AND
>     (cmp { build_real (TREE_TYPE (@0), dconst0); } @1))
>    /* x != NaN is always true, other ops are always false.  */
>    (if (REAL_VALUE_ISNAN (TREE_REAL_CST (@1))
> +    && (cmp == EQ_EXPR || cmp == NE_EXPR || !flag_trapping_math)
>    && !tree_expr_signaling_nan_p (@1)
>    && !tree_expr_maybe_signaling_nan_p (@0))
>     { constant_boolean_node (cmp == NE_EXPR, type); })
>    /* NaN != y is always true, other ops are always false.  */
>    (if (TREE_CODE (@0) == REAL_CST
>    && REAL_VALUE_ISNAN (TREE_REAL_CST (@0))
> +    && (cmp == EQ_EXPR || cmp == NE_EXPR || !flag_trapping_math)
>         && !tree_expr_signaling_nan_p (@0)
>         && !tree_expr_signaling_nan_p (@1))
>     { constant_boolean_node (cmp == NE_EXPR, type); })
> --- gcc/testsuite/c-c++-common/pr57371-4.c.jj    2022-04-28 15:56:07.137787247 +0200
> +++ gcc/testsuite/c-c++-common/pr57371-4.c    2022-12-02 13:34:03.076882811 +0100
> @@ -13,25 +13,25 @@ void nonfinite(unsigned short x) {
>   {
>     volatile int nonfinite_1;
>     nonfinite_1 = (float) x > QNAN;
> -    /* { dg-final { scan-tree-dump "nonfinite_1 = 0" "original" } } */
> +    /* { dg-final { scan-tree-dump "nonfinite_1 = \\(float\\)" "original" } } */
>   }
> 
>   {
>     volatile int nonfinite_2;
>     nonfinite_2 = (float) x >= QNAN;
> -    /* { dg-final { scan-tree-dump "nonfinite_2 = 0" "original" } } */
> +    /* { dg-final { scan-tree-dump "nonfinite_2 = \\(float\\)" "original" } } */
>   }
> 
>   {
>     volatile int nonfinite_3;
>     nonfinite_3 = (float) x < QNAN;
> -    /* { dg-final { scan-tree-dump "nonfinite_3 = 0" "original" } } */
> +    /* { dg-final { scan-tree-dump "nonfinite_3 = \\(float\\)" "original" } } */
>   }
> 
>   {
>     volatile int nonfinite_4;
>     nonfinite_4 = (float) x <= QNAN;
> -    /* { dg-final { scan-tree-dump "nonfinite_4 = 0" "original" } } */
> +    /* { dg-final { scan-tree-dump "nonfinite_4 = \\(float\\)" "original" } } */
>   }
> 
>   {
> --- gcc/testsuite/c-c++-common/pr57371-5.c.jj    2022-12-02 13:34:14.397714696 +0100
> +++ gcc/testsuite/c-c++-common/pr57371-5.c    2022-12-02 13:35:05.915949599 +0100
> @@ -0,0 +1,47 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -fno-signaling-nans -fno-trapping-math -fdump-tree-original" } */
> +
> +/* We can not get rid of comparison in tests below because of
> +   pending NaN exceptions.
> +
> +   TODO: avoid under -fno-trapping-math.  */
> +
> +#define QNAN __builtin_nanf ("0")
> +
> +void nonfinite(unsigned short x) {
> +  {
> +    volatile int nonfinite_1;
> +    nonfinite_1 = (float) x > QNAN;
> +    /* { dg-final { scan-tree-dump "nonfinite_1 = 0" "original" } } */
> +  }
> +
> +  {
> +    volatile int nonfinite_2;
> +    nonfinite_2 = (float) x >= QNAN;
> +    /* { dg-final { scan-tree-dump "nonfinite_2 = 0" "original" } } */
> +  }
> +
> +  {
> +    volatile int nonfinite_3;
> +    nonfinite_3 = (float) x < QNAN;
> +    /* { dg-final { scan-tree-dump "nonfinite_3 = 0" "original" } } */
> +  }
> +
> +  {
> +    volatile int nonfinite_4;
> +    nonfinite_4 = (float) x <= QNAN;
> +    /* { dg-final { scan-tree-dump "nonfinite_4 = 0" "original" } } */
> +  }
> +
> +  {
> +    volatile int nonfinite_11;
> +    nonfinite_11 = (float) x == QNAN;
> +    /* { dg-final { scan-tree-dump "nonfinite_11 = 0" "original" } } */
> +  }
> +
> +  {
> +    volatile int nonfinite_12;
> +    nonfinite_12 = (float) x != QNAN;
> +    /* { dg-final { scan-tree-dump "nonfinite_12 = 1" "original" } } */
> +  }
> +}
> --- gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-6.x.jj    2020-01-14 20:02:47.175603962 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-6.x    2022-12-05 10:44:10.037871848 +0100
> @@ -1,3 +1,4 @@
> +lappend additional_flags "-fno-trapping-math"
> # The ARM VxWorks kernel uses an external floating-point library in
> # which routines like __ledf2 are just aliases for __cmpdf2.  These
> # routines therefore don't handle NaNs correctly.
> --- gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-9.c.jj    2022-12-05 10:44:21.406705781 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-9.c    2022-12-05 10:44:57.237182398 +0100
> @@ -0,0 +1,31 @@
> +
> +const double dnan = 1.0/0.0 - 1.0/0.0;
> +double x = 1.0;
> +
> +extern void link_error (void);
> +extern void abort (void);
> +
> +main ()
> +{
> +#if ! defined (__vax__) && ! defined (_CRAY)
> +  /* NaN is an IEEE unordered operand.  All these test should be false.  */
> +  if (dnan == dnan)
> +    link_error ();
> +  if (dnan != x)
> +    x = 1.0;
> +  else
> +    link_error ();
> +
> +  if (dnan == x)
> +    link_error ();
> +#endif
> +  exit (0);
> +}
> +
> +#ifndef __OPTIMIZE__
> +void link_error (void)
> +{
> +  abort ();
> +}
> +#endif
> +
> --- gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-9.x.jj    2022-12-05 10:44:30.338575309 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-9.x    2022-12-05 10:44:32.991536565 +0100
> @@ -0,0 +1,16 @@
> +# The ARM VxWorks kernel uses an external floating-point library in
> +# which routines like __ledf2 are just aliases for __cmpdf2.  These
> +# routines therefore don't handle NaNs correctly.
> +if [istarget "arm*-*-vxworks*"] {
> +    set torture_eval_before_execute {
> +    global compiler_conditional_xfail_data
> +    set compiler_conditional_xfail_data {
> +        "The ARM kernel uses a flawed floating-point library."
> +        { "*-*-*" }
> +        { "-O0" }
> +        { "-mrtp" }
> +    }
> +    }
> +}
> +
> +return 0
> --- gcc/testsuite/gcc.dg/pr106805.c.jj    2022-12-02 13:02:06.813354371 +0100
> +++ gcc/testsuite/gcc.dg/pr106805.c    2022-12-02 13:11:42.844792390 +0100
> @@ -0,0 +1,29 @@
> +/* PR middle-end/106805 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -ftrapping-math" } */
> +/* { dg-add-options ieee } */
> +/* { dg-require-effective-target fenv_exceptions_double } */
> +
> +#include <fenv.h>
> +#include <stdlib.h>
> +
> +static inline int
> +foo (double x, double y)
> +{
> +  return x >= y && x <= y;
> +}
> +
> +int
> +main ()
> +{
> +  double x = __builtin_nan ("");
> +  volatile int r;
> +  r = foo (__builtin_nan (""), 1.0);
> +  if (!fetestexcept (FE_INVALID))
> +    abort ();
> +  feclearexcept (FE_ALL_EXCEPT);
> +  r = foo (x, __builtin_inf ());
> +  if (!fetestexcept (FE_INVALID))
> +    abort ();
> +  return 0;
> +}
> 
>    Jakub
>
  

Patch

--- gcc/match.pd.jj	2022-12-02 10:28:30.000000000 +0100
+++ gcc/match.pd	2022-12-02 12:50:28.701735269 +0100
@@ -5146,12 +5146,14 @@  (define_operator_list SYNC_FETCH_AND_AND
     (cmp { build_real (TREE_TYPE (@0), dconst0); } @1))
    /* x != NaN is always true, other ops are always false.  */
    (if (REAL_VALUE_ISNAN (TREE_REAL_CST (@1))
+	&& (cmp == EQ_EXPR || cmp == NE_EXPR || !flag_trapping_math)
 	&& !tree_expr_signaling_nan_p (@1)
 	&& !tree_expr_maybe_signaling_nan_p (@0))
     { constant_boolean_node (cmp == NE_EXPR, type); })
    /* NaN != y is always true, other ops are always false.  */
    (if (TREE_CODE (@0) == REAL_CST
 	&& REAL_VALUE_ISNAN (TREE_REAL_CST (@0))
+	&& (cmp == EQ_EXPR || cmp == NE_EXPR || !flag_trapping_math)
         && !tree_expr_signaling_nan_p (@0)
         && !tree_expr_signaling_nan_p (@1))
     { constant_boolean_node (cmp == NE_EXPR, type); })
--- gcc/testsuite/c-c++-common/pr57371-4.c.jj	2022-04-28 15:56:07.137787247 +0200
+++ gcc/testsuite/c-c++-common/pr57371-4.c	2022-12-02 13:34:03.076882811 +0100
@@ -13,25 +13,25 @@  void nonfinite(unsigned short x) {
   {
     volatile int nonfinite_1;
     nonfinite_1 = (float) x > QNAN;
-    /* { dg-final { scan-tree-dump "nonfinite_1 = 0" "original" } } */
+    /* { dg-final { scan-tree-dump "nonfinite_1 = \\(float\\)" "original" } } */
   }
 
   {
     volatile int nonfinite_2;
     nonfinite_2 = (float) x >= QNAN;
-    /* { dg-final { scan-tree-dump "nonfinite_2 = 0" "original" } } */
+    /* { dg-final { scan-tree-dump "nonfinite_2 = \\(float\\)" "original" } } */
   }
 
   {
     volatile int nonfinite_3;
     nonfinite_3 = (float) x < QNAN;
-    /* { dg-final { scan-tree-dump "nonfinite_3 = 0" "original" } } */
+    /* { dg-final { scan-tree-dump "nonfinite_3 = \\(float\\)" "original" } } */
   }
 
   {
     volatile int nonfinite_4;
     nonfinite_4 = (float) x <= QNAN;
-    /* { dg-final { scan-tree-dump "nonfinite_4 = 0" "original" } } */
+    /* { dg-final { scan-tree-dump "nonfinite_4 = \\(float\\)" "original" } } */
   }
 
   {
--- gcc/testsuite/c-c++-common/pr57371-5.c.jj	2022-12-02 13:34:14.397714696 +0100
+++ gcc/testsuite/c-c++-common/pr57371-5.c	2022-12-02 13:35:05.915949599 +0100
@@ -0,0 +1,47 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O -fno-signaling-nans -fno-trapping-math -fdump-tree-original" } */
+
+/* We can not get rid of comparison in tests below because of
+   pending NaN exceptions.
+
+   TODO: avoid under -fno-trapping-math.  */
+
+#define QNAN __builtin_nanf ("0")
+
+void nonfinite(unsigned short x) {
+  {
+    volatile int nonfinite_1;
+    nonfinite_1 = (float) x > QNAN;
+    /* { dg-final { scan-tree-dump "nonfinite_1 = 0" "original" } } */
+  }
+
+  {
+    volatile int nonfinite_2;
+    nonfinite_2 = (float) x >= QNAN;
+    /* { dg-final { scan-tree-dump "nonfinite_2 = 0" "original" } } */
+  }
+
+  {
+    volatile int nonfinite_3;
+    nonfinite_3 = (float) x < QNAN;
+    /* { dg-final { scan-tree-dump "nonfinite_3 = 0" "original" } } */
+  }
+
+  {
+    volatile int nonfinite_4;
+    nonfinite_4 = (float) x <= QNAN;
+    /* { dg-final { scan-tree-dump "nonfinite_4 = 0" "original" } } */
+  }
+
+  {
+    volatile int nonfinite_11;
+    nonfinite_11 = (float) x == QNAN;
+    /* { dg-final { scan-tree-dump "nonfinite_11 = 0" "original" } } */
+  }
+
+  {
+    volatile int nonfinite_12;
+    nonfinite_12 = (float) x != QNAN;
+    /* { dg-final { scan-tree-dump "nonfinite_12 = 1" "original" } } */
+  }
+}
--- gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-6.x.jj	2020-01-14 20:02:47.175603962 +0100
+++ gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-6.x	2022-12-05 10:44:10.037871848 +0100
@@ -1,3 +1,4 @@ 
+lappend additional_flags "-fno-trapping-math"
 # The ARM VxWorks kernel uses an external floating-point library in
 # which routines like __ledf2 are just aliases for __cmpdf2.  These
 # routines therefore don't handle NaNs correctly.
--- gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-9.c.jj	2022-12-05 10:44:21.406705781 +0100
+++ gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-9.c	2022-12-05 10:44:57.237182398 +0100
@@ -0,0 +1,31 @@ 
+
+const double dnan = 1.0/0.0 - 1.0/0.0;
+double x = 1.0;
+
+extern void link_error (void);
+extern void abort (void);
+
+main ()
+{
+#if ! defined (__vax__) && ! defined (_CRAY)
+  /* NaN is an IEEE unordered operand.  All these test should be false.  */
+  if (dnan == dnan)
+    link_error ();
+  if (dnan != x)
+    x = 1.0;
+  else
+    link_error ();
+
+  if (dnan == x)
+    link_error ();
+#endif
+  exit (0);
+}
+
+#ifndef __OPTIMIZE__
+void link_error (void)
+{
+  abort ();
+}
+#endif
+
--- gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-9.x.jj	2022-12-05 10:44:30.338575309 +0100
+++ gcc/testsuite/gcc.c-torture/execute/ieee/fp-cmp-9.x	2022-12-05 10:44:32.991536565 +0100
@@ -0,0 +1,16 @@ 
+# The ARM VxWorks kernel uses an external floating-point library in
+# which routines like __ledf2 are just aliases for __cmpdf2.  These
+# routines therefore don't handle NaNs correctly.
+if [istarget "arm*-*-vxworks*"] {
+    set torture_eval_before_execute {
+	global compiler_conditional_xfail_data
+	set compiler_conditional_xfail_data {
+	    "The ARM kernel uses a flawed floating-point library."
+	    { "*-*-*" }
+	    { "-O0" }
+	    { "-mrtp" }
+	}
+    }
+}
+
+return 0
--- gcc/testsuite/gcc.dg/pr106805.c.jj	2022-12-02 13:02:06.813354371 +0100
+++ gcc/testsuite/gcc.dg/pr106805.c	2022-12-02 13:11:42.844792390 +0100
@@ -0,0 +1,29 @@ 
+/* PR middle-end/106805 */
+/* { dg-do run } */
+/* { dg-options "-O2 -ftrapping-math" } */
+/* { dg-add-options ieee } */
+/* { dg-require-effective-target fenv_exceptions_double } */
+
+#include <fenv.h>
+#include <stdlib.h>
+
+static inline int
+foo (double x, double y)
+{
+  return x >= y && x <= y;
+}
+
+int
+main ()
+{
+  double x = __builtin_nan ("");
+  volatile int r;
+  r = foo (__builtin_nan (""), 1.0);
+  if (!fetestexcept (FE_INVALID))
+    abort ();
+  feclearexcept (FE_ALL_EXCEPT);
+  r = foo (x, __builtin_inf ());
+  if (!fetestexcept (FE_INVALID))
+    abort ();
+  return 0;
+}