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

[Richard Biener <rguenther@suse.de>]

> 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
>