Skip constant folding for fmin/max when either argument is sNaN [PR105414]

Hi,
   This patch skips constant folding for fmin/max when either argument
is sNaN. According to C standard,
   fmin(sNaN, sNaN）= qNaN, fmin(sNaN, NaN) = qNaN
   So signaling NaN should be tested and skipped for fmin/max in match.pd.

   Bootstrapped and tested on ppc64 Linux BE and LE with no regressions.
Is this okay for trunk? Any recommendations? Thanks a lot.

ChangeLog

2022-05-05 Haochen Gui <guihaoc@linux.ibm.com>

gcc/
	PR target/105414
	* match.pd (minmax): Skip constant folding for fmin/fmax when both
	arguments are sNaN or one is sNaN and another is NaN.

gcc/testsuite/
	PR target/105414
	* gcc.dg/pr105414.c: New.

patch.diff

On Thu, May 5, 2022 at 10:07 AM HAO CHEN GUI via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>    This patch skips constant folding for fmin/max when either argument
> is sNaN. According to C standard,
>    fmin(sNaN, sNaN）= qNaN, fmin(sNaN, NaN) = qNaN
>    So signaling NaN should be tested and skipped for fmin/max in match.pd.
>
>    Bootstrapped and tested on ppc64 Linux BE and LE with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.

OK.

Thanks,
Richard.

> ChangeLog
>
> 2022-05-05 Haochen Gui <guihaoc@linux.ibm.com>
>
> gcc/
>         PR target/105414
>         * match.pd (minmax): Skip constant folding for fmin/fmax when both
>         arguments are sNaN or one is sNaN and another is NaN.
>
> gcc/testsuite/
>         PR target/105414
>         * gcc.dg/pr105414.c: New.
>
> patch.diff
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index cad61848daa..f256bcbb483 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -3093,7 +3093,9 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  (for minmax (min max FMIN_ALL FMAX_ALL)
>   (simplify
>    (minmax @0 @0)
> -  @0))
> +  /* if both are sNaN, it should return qNaN.  */
> +  (if (!tree_expr_maybe_signaling_nan_p (@0))
> +    @0)))
>  /* min(max(x,y),y) -> y.  */
>  (simplify
>   (min:c (max:c @0 @1) @1)
> @@ -3193,12 +3195,13 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>     (minmax @1 (convert @2)))))
>
>  (for minmax (FMIN_ALL FMAX_ALL)
> - /* If either argument is NaN, return the other one.  Avoid the
> -    transformation if we get (and honor) a signalling NaN.  */
> + /* If either argument is NaN and other one is not sNaN, return the other
> +    one.  Avoid the transformation if we get (and honor) a signalling NaN.  */
>   (simplify
>    (minmax:c @0 REAL_CST@1)
> -  (if (real_isnan (TREE_REAL_CST_PTR (@1))
> -       && (!HONOR_SNANS (@1) || !TREE_REAL_CST (@1).signalling))
> +   (if (real_isnan (TREE_REAL_CST_PTR (@1))
> +       && (!HONOR_SNANS (@1) || !TREE_REAL_CST (@1).signalling)
> +       && !tree_expr_maybe_signaling_nan_p (@0))
>     @0)))
>  /* Convert fmin/fmax to MIN_EXPR/MAX_EXPR.  C99 requires these
>     functions to return the numeric arg if the other one is NaN.
> diff --git a/gcc/testsuite/gcc.dg/pr105414.c b/gcc/testsuite/gcc.dg/pr105414.c
> new file mode 100644
> index 00000000000..78772700acf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr105414.c
> @@ -0,0 +1,30 @@
> +/* { dg-do run { target { *-*-linux* *-*-gnu* } } } */
> +/* { dg-options "-O1 -fsignaling-nans -lm" } */
> +/* { dg-add-options ieee } */
> +/* { dg-require-effective-target issignaling } */
> +
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <math.h>
> +
> +int main()
> +{
> +  double a = __builtin_nans ("");
> +
> +  if (issignaling (fmin (a, a)))
> +    __builtin_abort ();
> +
> +  if (issignaling (fmax (a, a)))
> +    __builtin_abort ();
> +
> +  double b = __builtin_nan ("");
> +
> +  if (issignaling (fmin (a, b)))
> +    __builtin_abort ();
> +
> +  if (issignaling (fmax (a, b)))
> +    __builtin_abort ();
> +
> +  return 0;
> +}

on 2022/5/5 16:09, Richard Biener via Gcc-patches wrote:
> On Thu, May 5, 2022 at 10:07 AM HAO CHEN GUI via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Hi,
>>    This patch skips constant folding for fmin/max when either argument
>> is sNaN. According to C standard,
>>    fmin(sNaN, sNaN）= qNaN, fmin(sNaN, NaN) = qNaN
>>    So signaling NaN should be tested and skipped for fmin/max in match.pd.
>>
>>    Bootstrapped and tested on ppc64 Linux BE and LE with no regressions.
>> Is this okay for trunk? Any recommendations? Thanks a lot.
> 
> OK.
> 
> Thanks,
> Richard.
> 
>> ChangeLog
>>
>> 2022-05-05 Haochen Gui <guihaoc@linux.ibm.com>
>>
>> gcc/
>>         PR target/105414
>>         * match.pd (minmax): Skip constant folding for fmin/fmax when both
>>         arguments are sNaN or one is sNaN and another is NaN.
>>
>> gcc/testsuite/
>>         PR target/105414
>>         * gcc.dg/pr105414.c: New.
>>
>> patch.diff
>>
>> diff --git a/gcc/match.pd b/gcc/match.pd
>> index cad61848daa..f256bcbb483 100644
>> --- a/gcc/match.pd
>> +++ b/gcc/match.pd
>> @@ -3093,7 +3093,9 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>>  (for minmax (min max FMIN_ALL FMAX_ALL)
>>   (simplify
>>    (minmax @0 @0)
>> -  @0))
>> +  /* if both are sNaN, it should return qNaN.  */
>> +  (if (!tree_expr_maybe_signaling_nan_p (@0))
>> +    @0)))

Sorry for chiming in.

IIUC this patch is mainly for libc function fmin/fmax and the iterator here
covers min/max and fmin/fmax.  I wonder if it's intent to make this change
for min/max as well?

As tree.def, "if either operand is NaN, then it is unspecified", the optimization
for min/max seems still acceptable?

BR,
Kewen

On Thu, May 5, 2022 at 10:30 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> on 2022/5/5 16:09, Richard Biener via Gcc-patches wrote:
> > On Thu, May 5, 2022 at 10:07 AM HAO CHEN GUI via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> Hi,
> >>    This patch skips constant folding for fmin/max when either argument
> >> is sNaN. According to C standard,
> >>    fmin(sNaN, sNaN）= qNaN, fmin(sNaN, NaN) = qNaN
> >>    So signaling NaN should be tested and skipped for fmin/max in match.pd.
> >>
> >>    Bootstrapped and tested on ppc64 Linux BE and LE with no regressions.
> >> Is this okay for trunk? Any recommendations? Thanks a lot.
> >
> > OK.
> >
> > Thanks,
> > Richard.
> >
> >> ChangeLog
> >>
> >> 2022-05-05 Haochen Gui <guihaoc@linux.ibm.com>
> >>
> >> gcc/
> >>         PR target/105414
> >>         * match.pd (minmax): Skip constant folding for fmin/fmax when both
> >>         arguments are sNaN or one is sNaN and another is NaN.
> >>
> >> gcc/testsuite/
> >>         PR target/105414
> >>         * gcc.dg/pr105414.c: New.
> >>
> >> patch.diff
> >>
> >> diff --git a/gcc/match.pd b/gcc/match.pd
> >> index cad61848daa..f256bcbb483 100644
> >> --- a/gcc/match.pd
> >> +++ b/gcc/match.pd
> >> @@ -3093,7 +3093,9 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >>  (for minmax (min max FMIN_ALL FMAX_ALL)
> >>   (simplify
> >>    (minmax @0 @0)
> >> -  @0))
> >> +  /* if both are sNaN, it should return qNaN.  */
> >> +  (if (!tree_expr_maybe_signaling_nan_p (@0))
> >> +    @0)))
>
> Sorry for chiming in.
>
> IIUC this patch is mainly for libc function fmin/fmax and the iterator here
> covers min/max and fmin/fmax.  I wonder if it's intent to make this change
> for min/max as well?
>
> As tree.def, "if either operand is NaN, then it is unspecified", the optimization
> for min/max seems still acceptable?

MIN/MAX_EXPR shouldn't even appear with -fsignalling-nans for this
reason, at least that's what I thought.  But yes, you might have a point
here (but maybe it's also not strictly enough specified).  One option would
be to do (minmax == MAX_EXPR || minmax == MIN_EXPR || !tree_expr ...)

Joseph - are MIN_EXPR and MAX_EXPR supposed to turn sNaN into qNaN
and the 'undefinedness' is merely as to which operand is chosen?

Richard.

>
> BR,
> Kewen

On 5/5/2022 下午 4:30, Kewen.Lin wrote:
> on 2022/5/5 16:09, Richard Biener via Gcc-patches wrote:
>> On Thu, May 5, 2022 at 10:07 AM HAO CHEN GUI via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>> Hi,
>>>    This patch skips constant folding for fmin/max when either argument
>>> is sNaN. According to C standard,
>>>    fmin(sNaN, sNaN）= qNaN, fmin(sNaN, NaN) = qNaN
>>>    So signaling NaN should be tested and skipped for fmin/max in match.pd.
>>>
>>>    Bootstrapped and tested on ppc64 Linux BE and LE with no regressions.
>>> Is this okay for trunk? Any recommendations? Thanks a lot.
>>
>> OK.
>>
>> Thanks,
>> Richard.
>>
>>> ChangeLog
>>>
>>> 2022-05-05 Haochen Gui <guihaoc@linux.ibm.com>
>>>
>>> gcc/
>>>         PR target/105414
>>>         * match.pd (minmax): Skip constant folding for fmin/fmax when both
>>>         arguments are sNaN or one is sNaN and another is NaN.
>>>
>>> gcc/testsuite/
>>>         PR target/105414
>>>         * gcc.dg/pr105414.c: New.
>>>
>>> patch.diff
>>>
>>> diff --git a/gcc/match.pd b/gcc/match.pd
>>> index cad61848daa..f256bcbb483 100644
>>> --- a/gcc/match.pd
>>> +++ b/gcc/match.pd
>>> @@ -3093,7 +3093,9 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>>>  (for minmax (min max FMIN_ALL FMAX_ALL)
>>>   (simplify
>>>    (minmax @0 @0)
>>> -  @0))
>>> +  /* if both are sNaN, it should return qNaN.  */
>>> +  (if (!tree_expr_maybe_signaling_nan_p (@0))
>>> +    @0)))
> 
> Sorry for chiming in.
> 
> IIUC this patch is mainly for libc function fmin/fmax and the iterator here
> covers min/max and fmin/fmax.  I wonder if it's intent to make this change
> for min/max as well?
> 
> As tree.def, "if either operand is NaN, then it is unspecified", the optimization
> for min/max seems still acceptable?

For MIN/MAX_EXPR, the result is undefined with NaN. So I think we shouldn't do
constant folding. We should let target decide how to deal with it. The "undefined"
here means the result depends on targets as far as I understand.
> 
> BR,
> Kewen

Hi!

On Thu, May 05, 2022 at 05:30:58PM +0800, HAO CHEN GUI wrote:
> On 5/5/2022 下午 4:30, Kewen.Lin wrote:
> > on 2022/5/5 16:09, Richard Biener via Gcc-patches wrote:
> >> On Thu, May 5, 2022 at 10:07 AM HAO CHEN GUI via Gcc-patches
> >> <gcc-patches@gcc.gnu.org> wrote:
> >>>    This patch skips constant folding for fmin/max when either argument
> >>> is sNaN. According to C standard,
> >>>    fmin(sNaN, sNaN）= qNaN, fmin(sNaN, NaN) = qNaN
> >>>    So signaling NaN should be tested and skipped for fmin/max in match.pd.
> >>>
> >>>    Bootstrapped and tested on ppc64 Linux BE and LE with no regressions.

The C standard does not talk about sNaNs *at all*, in any released
version of the standard.  And the C2x drafts do not talk about signaling
arguments for fmin/fmax specifically, so it should just raise an error
like any other floating operation with an sNaN arg will.  This means we
have to make sure to not optimise away all operations if there may be
an sNaN (and we have HONOR_SNANS for the mode in use).

You never have to convert to a qNaN manually.   Instead, any normal
operation on the sNaN will raise the exception, and convert to the qNaN.
There is no sane way you can raise the exception manually, so you should
make sure we end up with a real operation in the RTL, and then generate
proper machine code for it as well.  See rs6000 extendsfdf2 for example,
for that last part.

And of course both the gimple min_expr and the RTL smin are not defined
for NaN inputs at all anyway :-P


Segher

On Thu, 5 May 2022, Richard Biener via Gcc-patches wrote:

> MIN/MAX_EXPR shouldn't even appear with -fsignalling-nans for this
> reason, at least that's what I thought.  But yes, you might have a point
> here (but maybe it's also not strictly enough specified).  One option would
> be to do (minmax == MAX_EXPR || minmax == MIN_EXPR || !tree_expr ...)
> 
> Joseph - are MIN_EXPR and MAX_EXPR supposed to turn sNaN into qNaN
> and the 'undefinedness' is merely as to which operand is chosen?

I don't know what MIN_EXPR and MAX_EXPR are supposed to do with sNaN 
arguments.  As noted, the fmax and fmin functions should produce a qNaN 
result with the "invalid" exception raised (so with an sNaN argument, it's 
definitely not valid to fold them in the absence of -fno-trapping-math; 
with -fsignaling-nans -fno-trapping-math, if an argument is known to be 
sNaN it would be valid to fold to qNaN, but I doubt there's much use of 
that option combination).

C never attempts to define which qNaN result (choice of payload or sign 
bit) is produced by an operation and I don't think our optimizations 
should be trying to define that (with any command-line options currently 
supported) either (other than for non-computational operations such as 
fabs and copysign, but even there there is scope for 
implementation-defined handling of assignment as a convertFormat operation 
rather than a copy operation).  Note that while some architectures 
propagate NaN payloads from a NaN operand to an instruction, others (e.g. 
RISC-V) do not, and when payloads are propagated there would still be the 
matter of which payload is chosen when there is more than one NaN operand 
(on x86, that is handled differently for SSE and x87 instructions).

On Thu, May 5, 2022 at 7:11 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Thu, 5 May 2022, Richard Biener via Gcc-patches wrote:
>
> > MIN/MAX_EXPR shouldn't even appear with -fsignalling-nans for this
> > reason, at least that's what I thought.  But yes, you might have a point
> > here (but maybe it's also not strictly enough specified).  One option would
> > be to do (minmax == MAX_EXPR || minmax == MIN_EXPR || !tree_expr ...)
> >
> > Joseph - are MIN_EXPR and MAX_EXPR supposed to turn sNaN into qNaN
> > and the 'undefinedness' is merely as to which operand is chosen?
>
> I don't know what MIN_EXPR and MAX_EXPR are supposed to do with sNaN
> arguments.  As noted, the fmax and fmin functions should produce a qNaN
> result with the "invalid" exception raised (so with an sNaN argument, it's
> definitely not valid to fold them in the absence of -fno-trapping-math;
> with -fsignaling-nans -fno-trapping-math, if an argument is known to be
> sNaN it would be valid to fold to qNaN, but I doubt there's much use of
> that option combination).
>
> C never attempts to define which qNaN result (choice of payload or sign
> bit) is produced by an operation and I don't think our optimizations
> should be trying to define that (with any command-line options currently
> supported) either (other than for non-computational operations such as
> fabs and copysign, but even there there is scope for
> implementation-defined handling of assignment as a convertFormat operation
> rather than a copy operation).  Note that while some architectures
> propagate NaN payloads from a NaN operand to an instruction, others (e.g.
> RISC-V) do not, and when payloads are propagated there would still be the
> matter of which payload is chosen when there is more than one NaN operand
> (on x86, that is handled differently for SSE and x87 instructions).

Thanks.  So I think the patch as posted is correct on the safe side in
letting the CPU to decide what happens with sNaNs.  As said, the chance
seeing an sNaN and MAX/MIN_EXPR is low.

So I'm defering to the poster if he wants to re-post with treating MIN/MAX_EXPR
special in the optimistic sense.

Richard.


> --
> Joseph S. Myers
> joseph@codesourcery.com