PR middle-end/111701: signbit(x*x) vs -fsignaling-nans

  This patch addresses PR middle-end/111701 where optimization of signbit(x*x)
using tree_nonnegative_p incorrectly eliminates a floating point
multiplication when the operands may potentially be signaling NaNs.

The above bug fix also provides a solution or work-around to the tricky
issue in PR middle-end/111701, that the results of IEEE operations on NaNs
are specified to return a NaN result, but fail to (precisely) specify
the exact NaN representation of this result.  Hence for the operation
"-NaN*-NaN" different hardware implementations (targets) return different
results.  Ultimately knowing what the resulting NaN "payload" of an
operation is can only be known by executing that operation at run-time,
and I'd suggest that GCC's -fsignaling-nans provides a mechanism for
handling code that uses NaN representations for communication/signaling
(which is a different but related concept to IEEE's sNaN).

One nice thing about this patch, which may or may not be a P2 regression
fix, is that it only affects (improves) code compiled with -fsignaling-nans
so should be extremely safe even for this point in stage 3.

This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
and make -k check, both with and without --target_board=unix{-m32}
with no new failures.  Ok for mainline?


2024-04-26  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog
        PR middle-end/111701
        * fold-const.cc (tree_binary_nonnegative_warnv_p) <case MULT_EXPR>:
        Split handling of floating point and integer types.  For equal
        floating point operands, avoid optimization if the operand may be
        a signaling NaN.

gcc/testsuite/ChangeLog
        PR middle-end/111701
        * gcc.dg/pr111701-1.c: New test case.
        * gcc.dg/pr111701-2.c: Likewise.


Thanks in advance,
Roger
--
  

On Fri, Apr 26, 2024 at 10:19 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> This patch addresses PR middle-end/111701 where optimization of signbit(x*x)
> using tree_nonnegative_p incorrectly eliminates a floating point
> multiplication when the operands may potentially be signaling NaNs.
>
> The above bug fix also provides a solution or work-around to the tricky
> issue in PR middle-end/111701, that the results of IEEE operations on NaNs
> are specified to return a NaN result, but fail to (precisely) specify
> the exact NaN representation of this result.  Hence for the operation
> "-NaN*-NaN" different hardware implementations (targets) return different
> results.  Ultimately knowing what the resulting NaN "payload" of an
> operation is can only be known by executing that operation at run-time,
> and I'd suggest that GCC's -fsignaling-nans provides a mechanism for
> handling code that uses NaN representations for communication/signaling
> (which is a different but related concept to IEEE's sNaN).
>
> One nice thing about this patch, which may or may not be a P2 regression
> fix, is that it only affects (improves) code compiled with -fsignaling-nans
> so should be extremely safe even for this point in stage 3.
>
> This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> and make -k check, both with and without --target_board=unix{-m32}
> with no new failures.  Ok for mainline?

Hmm, but the bugreports are not about sNaN but about the fact that
the sign of the NaN produced by 0/0 or by -NaN*-NaN is not well-defined.
So I don't think this is the correct approach to fix this.  We'd instead
have to use tree_expr_maybe_nan_p () - and if NaN*NaN cannot be
-NaN (is that at least specified?) then the RECURSE path should
still work as well.

Richard.

>
> 2024-04-26  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>         PR middle-end/111701
>         * fold-const.cc (tree_binary_nonnegative_warnv_p) <case MULT_EXPR>:
>         Split handling of floating point and integer types.  For equal
>         floating point operands, avoid optimization if the operand may be
>         a signaling NaN.
>
> gcc/testsuite/ChangeLog
>         PR middle-end/111701
>         * gcc.dg/pr111701-1.c: New test case.
>         * gcc.dg/pr111701-2.c: Likewise.
>
>
> Thanks in advance,
> Roger
> --
>
  

> From: Richard Biener <richard.guenther@gmail.com>
> On Fri, Apr 26, 2024 at 10:19 AM Roger Sayle <roger@nextmovesoftware.com>
> wrote:
> >
> > This patch addresses PR middle-end/111701 where optimization of
> > signbit(x*x) using tree_nonnegative_p incorrectly eliminates a
> > floating point multiplication when the operands may potentially be signaling
> NaNs.
> >
> > The above bug fix also provides a solution or work-around to the
> > tricky issue in PR middle-end/111701, that the results of IEEE
> > operations on NaNs are specified to return a NaN result, but fail to
> > (precisely) specify the exact NaN representation of this result.
> > Hence for the operation "-NaN*-NaN" different hardware implementations
> > (targets) return different results.  Ultimately knowing what the
> > resulting NaN "payload" of an operation is can only be known by
> > executing that operation at run-time, and I'd suggest that GCC's
> > -fsignaling-nans provides a mechanism for handling code that uses NaN
> > representations for communication/signaling (which is a different but related
> concept to IEEE's sNaN).
> >
> > One nice thing about this patch, which may or may not be a P2
> > regression fix, is that it only affects (improves) code compiled with
> > -fsignaling-nans so should be extremely safe even for this point in stage 3.
> >
> > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> > and make -k check, both with and without --target_board=unix{-m32}
> > with no new failures.  Ok for mainline?
> 
> Hmm, but the bugreports are not about sNaN but about the fact that the sign of
> the NaN produced by 0/0 or by -NaN*-NaN is not well-defined.
> So I don't think this is the correct approach to fix this.  We'd instead have to use
> tree_expr_maybe_nan_p () - and if NaN*NaN cannot be -NaN (is that at least
> specified?) then the RECURSE path should still work as well.

If we ignore the bugzilla PR for now, can we agree that if x is a signaling NaN,
that we shouldn't be eliminating x*x?  i.e. that this patch fixes a real bug, but
perhaps not (precisely) the one described in PR middle-end/111701.

Once the signaling NaN case is correctly handled, the use of -fsignaling-nans
can be used as a workaround for PR 111701, allowing it to perhaps be reduced
from a P2 to a P3 regression (or even not a bug if the qNaN case is undefined behavior).
When I wrote this patch I was trying to help with GCC 14's stage 3.
 
> > 2024-04-26  Roger Sayle  <roger@nextmovesoftware.com>
> >
> > gcc/ChangeLog
> >         PR middle-end/111701
> >         * fold-const.cc (tree_binary_nonnegative_warnv_p) <case MULT_EXPR>:
> >         Split handling of floating point and integer types.  For equal
> >         floating point operands, avoid optimization if the operand may be
> >         a signaling NaN.
> >
> > gcc/testsuite/ChangeLog
> >         PR middle-end/111701
> >         * gcc.dg/pr111701-1.c: New test case.
> >         * gcc.dg/pr111701-2.c: Likewise.
> >
  

On Thu, May 2, 2024 at 11:34 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> > From: Richard Biener <richard.guenther@gmail.com>
> > On Fri, Apr 26, 2024 at 10:19 AM Roger Sayle <roger@nextmovesoftware.com>
> > wrote:
> > >
> > > This patch addresses PR middle-end/111701 where optimization of
> > > signbit(x*x) using tree_nonnegative_p incorrectly eliminates a
> > > floating point multiplication when the operands may potentially be signaling
> > NaNs.
> > >
> > > The above bug fix also provides a solution or work-around to the
> > > tricky issue in PR middle-end/111701, that the results of IEEE
> > > operations on NaNs are specified to return a NaN result, but fail to
> > > (precisely) specify the exact NaN representation of this result.
> > > Hence for the operation "-NaN*-NaN" different hardware implementations
> > > (targets) return different results.  Ultimately knowing what the
> > > resulting NaN "payload" of an operation is can only be known by
> > > executing that operation at run-time, and I'd suggest that GCC's
> > > -fsignaling-nans provides a mechanism for handling code that uses NaN
> > > representations for communication/signaling (which is a different but related
> > concept to IEEE's sNaN).
> > >
> > > One nice thing about this patch, which may or may not be a P2
> > > regression fix, is that it only affects (improves) code compiled with
> > > -fsignaling-nans so should be extremely safe even for this point in stage 3.
> > >
> > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> > > and make -k check, both with and without --target_board=unix{-m32}
> > > with no new failures.  Ok for mainline?
> >
> > Hmm, but the bugreports are not about sNaN but about the fact that the sign of
> > the NaN produced by 0/0 or by -NaN*-NaN is not well-defined.
> > So I don't think this is the correct approach to fix this.  We'd instead have to use
> > tree_expr_maybe_nan_p () - and if NaN*NaN cannot be -NaN (is that at least
> > specified?) then the RECURSE path should still work as well.
>
> If we ignore the bugzilla PR for now, can we agree that if x is a signaling NaN,
> that we shouldn't be eliminating x*x?  i.e. that this patch fixes a real bug, but
> perhaps not (precisely) the one described in PR middle-end/111701.

This might or might not be covered by -fdelete-dead-exceptions - at least in
the past we were OK with removing traps like for -ftrapv (-ftrapv makes
signed overflow no longer invoke undefined behavior) or when deleting
loads that might trap (but those would invoke undefined behavior).

I bet the C standard doesn't say anything about sNaNs or how an operation
with it has to behave in the abstract machine.  We do document though
that it "disables optimizations that may change the number of
exceptions visible with
signaling NaNs" which suggests that with -fsignaling-nans we have to preserve
all such traps but I am very sure DCE will simply elide unused ops here
(also for other FP operations with -ftrapping-math - but there we do
not document
that we preserve all traps).

With the patch the multiplication is only preserved because __builtin_signbit
still uses it.  A plain

void foo(double x)
{
   x*x;
}

has the multiplication elided during gimplification already (even at -O0).

So I don't think the patch is a meaningful improvement as to preserve
multiplications of sNaNs.

Richard.

> Once the signaling NaN case is correctly handled, the use of -fsignaling-nans
> can be used as a workaround for PR 111701, allowing it to perhaps be reduced
> from a P2 to a P3 regression (or even not a bug if the qNaN case is undefined behavior).
> When I wrote this patch I was trying to help with GCC 14's stage 3.
>
> > > 2024-04-26  Roger Sayle  <roger@nextmovesoftware.com>
> > >
> > > gcc/ChangeLog
> > >         PR middle-end/111701
> > >         * fold-const.cc (tree_binary_nonnegative_warnv_p) <case MULT_EXPR>:
> > >         Split handling of floating point and integer types.  For equal
> > >         floating point operands, avoid optimization if the operand may be
> > >         a signaling NaN.
> > >
> > > gcc/testsuite/ChangeLog
> > >         PR middle-end/111701
> > >         * gcc.dg/pr111701-1.c: New test case.
> > >         * gcc.dg/pr111701-2.c: Likewise.
> > >
>
>
  

> From: Richard Biener <richard.guenther@gmail.com>
> On Thu, May 2, 2024 at 11:34 AM Roger Sayle <roger@nextmovesoftware.com>
> wrote:
> >
> >
> > > From: Richard Biener <richard.guenther@gmail.com> On Fri, Apr 26,
> > > 2024 at 10:19 AM Roger Sayle <roger@nextmovesoftware.com>
> > > wrote:
> > > >
> > > > This patch addresses PR middle-end/111701 where optimization of
> > > > signbit(x*x) using tree_nonnegative_p incorrectly eliminates a
> > > > floating point multiplication when the operands may potentially be
> > > > signaling
> > > NaNs.
> > > >
> > > > The above bug fix also provides a solution or work-around to the
> > > > tricky issue in PR middle-end/111701, that the results of IEEE
> > > > operations on NaNs are specified to return a NaN result, but fail
> > > > to
> > > > (precisely) specify the exact NaN representation of this result.
> > > > Hence for the operation "-NaN*-NaN" different hardware
> > > > implementations
> > > > (targets) return different results.  Ultimately knowing what the
> > > > resulting NaN "payload" of an operation is can only be known by
> > > > executing that operation at run-time, and I'd suggest that GCC's
> > > > -fsignaling-nans provides a mechanism for handling code that uses
> > > > NaN representations for communication/signaling (which is a
> > > > different but related
> > > concept to IEEE's sNaN).
> > > >
> > > > One nice thing about this patch, which may or may not be a P2
> > > > regression fix, is that it only affects (improves) code compiled
> > > > with -fsignaling-nans so should be extremely safe even for this point in stage
> 3.
> > > >
> > > > This patch has been tested on x86_64-pc-linux-gnu with make
> > > > bootstrap and make -k check, both with and without
> > > > --target_board=unix{-m32} with no new failures.  Ok for mainline?
> > >
> > > Hmm, but the bugreports are not about sNaN but about the fact that
> > > the sign of the NaN produced by 0/0 or by -NaN*-NaN is not well-defined.
> > > So I don't think this is the correct approach to fix this.  We'd
> > > instead have to use tree_expr_maybe_nan_p () - and if NaN*NaN cannot
> > > be -NaN (is that at least
> > > specified?) then the RECURSE path should still work as well.
> >
> > If we ignore the bugzilla PR for now, can we agree that if x is a
> > signaling NaN, that we shouldn't be eliminating x*x?  i.e. that this
> > patch fixes a real bug, but perhaps not (precisely) the one described in PR
> middle-end/111701.
> 
> This might or might not be covered by -fdelete-dead-exceptions - at least in the
> past we were OK with removing traps like for -ftrapv (-ftrapv makes signed
> overflow no longer invoke undefined behavior) or when deleting loads that might
> trap (but those would invoke undefined behavior).
> 
> I bet the C standard doesn't say anything about sNaNs or how an operation with
> it has to behave in the abstract machine.  We do document though that it
> "disables optimizations that may change the number of exceptions visible with
> signaling NaNs" which suggests that with -fsignaling-nans we have to preserve all
> such traps but I am very sure DCE will simply elide unused ops here (also for other
> FP operations with -ftrapping-math - but there we do not document that we
> preserve all traps).
> 
> With the patch the multiplication is only preserved because __builtin_signbit still
> uses it.  A plain
> 
> void foo(double x)
> {
>    x*x;
> }
> 
> has the multiplication elided during gimplification already (even at -O0).

void foo(double x)
{
  double t = x*x;
}

when compiled with -fsignaling-nans -fexceptions -fnon-call-exceptions
doesn't exhibit the above bug.  Perhaps this short-coming of gimplification
deserves its own Bugzilla PR?
 
> So I don't think the patch is a meaningful improvement as to preserve
> multiplications of sNaNs.
> 
> Richard.
> 
> > Once the signaling NaN case is correctly handled, the use of
> > -fsignaling-nans can be used as a workaround for PR 111701, allowing
> > it to perhaps be reduced from a P2 to a P3 regression (or even not a bug if the
> qNaN case is undefined behavior).
> > When I wrote this patch I was trying to help with GCC 14's stage 3.
> >
> > > > 2024-04-26  Roger Sayle  <roger@nextmovesoftware.com>
> > > >
> > > > gcc/ChangeLog
> > > >         PR middle-end/111701
> > > >         * fold-const.cc (tree_binary_nonnegative_warnv_p) <case
> MULT_EXPR>:
> > > >         Split handling of floating point and integer types.  For equal
> > > >         floating point operands, avoid optimization if the operand may be
> > > >         a signaling NaN.
> > > >
> > > > gcc/testsuite/ChangeLog
> > > >         PR middle-end/111701
> > > >         * gcc.dg/pr111701-1.c: New test case.
> > > >         * gcc.dg/pr111701-2.c: Likewise.
> > > >
> >
> >
  

On Thu, May 2, 2024 at 3:48 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> > From: Richard Biener <richard.guenther@gmail.com>
> > On Thu, May 2, 2024 at 11:34 AM Roger Sayle <roger@nextmovesoftware.com>
> > wrote:
> > >
> > >
> > > > From: Richard Biener <richard.guenther@gmail.com> On Fri, Apr 26,
> > > > 2024 at 10:19 AM Roger Sayle <roger@nextmovesoftware.com>
> > > > wrote:
> > > > >
> > > > > This patch addresses PR middle-end/111701 where optimization of
> > > > > signbit(x*x) using tree_nonnegative_p incorrectly eliminates a
> > > > > floating point multiplication when the operands may potentially be
> > > > > signaling
> > > > NaNs.
> > > > >
> > > > > The above bug fix also provides a solution or work-around to the
> > > > > tricky issue in PR middle-end/111701, that the results of IEEE
> > > > > operations on NaNs are specified to return a NaN result, but fail
> > > > > to
> > > > > (precisely) specify the exact NaN representation of this result.
> > > > > Hence for the operation "-NaN*-NaN" different hardware
> > > > > implementations
> > > > > (targets) return different results.  Ultimately knowing what the
> > > > > resulting NaN "payload" of an operation is can only be known by
> > > > > executing that operation at run-time, and I'd suggest that GCC's
> > > > > -fsignaling-nans provides a mechanism for handling code that uses
> > > > > NaN representations for communication/signaling (which is a
> > > > > different but related
> > > > concept to IEEE's sNaN).
> > > > >
> > > > > One nice thing about this patch, which may or may not be a P2
> > > > > regression fix, is that it only affects (improves) code compiled
> > > > > with -fsignaling-nans so should be extremely safe even for this point in stage
> > 3.
> > > > >
> > > > > This patch has been tested on x86_64-pc-linux-gnu with make
> > > > > bootstrap and make -k check, both with and without
> > > > > --target_board=unix{-m32} with no new failures.  Ok for mainline?
> > > >
> > > > Hmm, but the bugreports are not about sNaN but about the fact that
> > > > the sign of the NaN produced by 0/0 or by -NaN*-NaN is not well-defined.
> > > > So I don't think this is the correct approach to fix this.  We'd
> > > > instead have to use tree_expr_maybe_nan_p () - and if NaN*NaN cannot
> > > > be -NaN (is that at least
> > > > specified?) then the RECURSE path should still work as well.
> > >
> > > If we ignore the bugzilla PR for now, can we agree that if x is a
> > > signaling NaN, that we shouldn't be eliminating x*x?  i.e. that this
> > > patch fixes a real bug, but perhaps not (precisely) the one described in PR
> > middle-end/111701.
> >
> > This might or might not be covered by -fdelete-dead-exceptions - at least in the
> > past we were OK with removing traps like for -ftrapv (-ftrapv makes signed
> > overflow no longer invoke undefined behavior) or when deleting loads that might
> > trap (but those would invoke undefined behavior).
> >
> > I bet the C standard doesn't say anything about sNaNs or how an operation with
> > it has to behave in the abstract machine.  We do document though that it
> > "disables optimizations that may change the number of exceptions visible with
> > signaling NaNs" which suggests that with -fsignaling-nans we have to preserve all
> > such traps but I am very sure DCE will simply elide unused ops here (also for other
> > FP operations with -ftrapping-math - but there we do not document that we
> > preserve all traps).
> >
> > With the patch the multiplication is only preserved because __builtin_signbit still
> > uses it.  A plain
> >
> > void foo(double x)
> > {
> >    x*x;
> > }
> >
> > has the multiplication elided during gimplification already (even at -O0).
>
> void foo(double x)
> {
>   double t = x*x;
> }
>
> when compiled with -fsignaling-nans -fexceptions -fnon-call-exceptions
> doesn't exhibit the above bug.  Perhaps this short-coming of gimplification
> deserves its own Bugzilla PR?

With optimization you need -fno-delete-dead-exceptions to preserve the
multiply.  Btw, the observable trap is there even without -fnon-call-exceptions
and a trap isn't an exception.

So what I do not necessarily agree with is that we need to preserve
the multiplication with -fsignaling-nans.  Do we consider a program doing

handler() { exit(0); }

 x = sNaN;
...
 sigaction(SIGFPE, ... handler)
 x*x;
 format_hard_drive();

and expecting the program to exit(0) rather than formating the hard-disk
to be expecting something the C standard guarantees?  And is it enough
for the program to enable -fsignaling-nans for this?

If so then the first and foremost bug is that 'x*x' doesn't have
TREE_SIDE_EFFECTS
set and thus we do not preserve it when optimizing __builtin_signbit () of it.

Richard.

>
> > So I don't think the patch is a meaningful improvement as to preserve
> > multiplications of sNaNs.
> >
> > Richard.
> >
> > > Once the signaling NaN case is correctly handled, the use of
> > > -fsignaling-nans can be used as a workaround for PR 111701, allowing
> > > it to perhaps be reduced from a P2 to a P3 regression (or even not a bug if the
> > qNaN case is undefined behavior).
> > > When I wrote this patch I was trying to help with GCC 14's stage 3.
> > >
> > > > > 2024-04-26  Roger Sayle  <roger@nextmovesoftware.com>
> > > > >
> > > > > gcc/ChangeLog
> > > > >         PR middle-end/111701
> > > > >         * fold-const.cc (tree_binary_nonnegative_warnv_p) <case
> > MULT_EXPR>:
> > > > >         Split handling of floating point and integer types.  For equal
> > > > >         floating point operands, avoid optimization if the operand may be
> > > > >         a signaling NaN.
> > > > >
> > > > > gcc/testsuite/ChangeLog
> > > > >         PR middle-end/111701
> > > > >         * gcc.dg/pr111701-1.c: New test case.
> > > > >         * gcc.dg/pr111701-2.c: Likewise.
> > > > >
> > >
> > >
>
  

On Fri, 3 May 2024, Richard Biener wrote:

> So what I do not necessarily agree with is that we need to preserve
> the multiplication with -fsignaling-nans.  Do we consider a program doing
> 
> handler() { exit(0); }
> 
>  x = sNaN;
> ...
>  sigaction(SIGFPE, ... handler)
>  x*x;
>  format_hard_drive();
> 
> and expecting the program to exit(0) rather than formating the hard-disk
> to be expecting something the C standard guarantees?  And is it enough
> for the program to enable -fsignaling-nans for this?
> 
> If so then the first and foremost bug is that 'x*x' doesn't have
> TREE_SIDE_EFFECTS
> set and thus we do not preserve it when optimizing __builtin_signbit () of it.

Signaling NaNs don't seem relevant here.  "Signal" means "set the 
exception flag" - and 0 * Inf raises the same "invalid" exception flag as 
sNaN * sNaN.  Changing flow of control on an exception is outside the 
scope of standard C and requires nonstandard extensions such as 
feenableexcept.  (At present -ftrapping-math covers both kinds of 
exception handling - the default setting of a flag, and the nonstandard 
change of flow of control.)
  

On Tue, May 7, 2024 at 10:44 PM Joseph Myers <josmyers@redhat.com> wrote:
>
> On Fri, 3 May 2024, Richard Biener wrote:
>
> > So what I do not necessarily agree with is that we need to preserve
> > the multiplication with -fsignaling-nans.  Do we consider a program doing
> >
> > handler() { exit(0); }
> >
> >  x = sNaN;
> > ...
> >  sigaction(SIGFPE, ... handler)
> >  x*x;
> >  format_hard_drive();
> >
> > and expecting the program to exit(0) rather than formating the hard-disk
> > to be expecting something the C standard guarantees?  And is it enough
> > for the program to enable -fsignaling-nans for this?
> >
> > If so then the first and foremost bug is that 'x*x' doesn't have
> > TREE_SIDE_EFFECTS
> > set and thus we do not preserve it when optimizing __builtin_signbit () of it.
>
> Signaling NaNs don't seem relevant here.  "Signal" means "set the
> exception flag" - and 0 * Inf raises the same "invalid" exception flag as
> sNaN * sNaN.  Changing flow of control on an exception is outside the
> scope of standard C and requires nonstandard extensions such as
> feenableexcept.  (At present -ftrapping-math covers both kinds of
> exception handling - the default setting of a flag, and the nonstandard
> change of flow of control.)

So it's reasonable to require -fnon-call-exceptions (which now enables
-fexceptions) and -fno-delete-dead-exceptions to have GCC preserve
a change of control flow side-effect of x*x?  We do not preserve
FP exception bits set by otherwise unused operations, that is, we
do not consider that side-effect to be observable even with
-ftrapping-math.  In fact I most uses of flag_trapping_math
are related to a possible control flow side-effect of FP math.
Exact preservation of FP exception flags will likely have to disable
all FP optimization if one considers FE_INEXACT and FE_UNDERFLOW.

Every time I try to make up my mind how to improve the situation for
the user I'm only confusing myself :/

Richard.

> --
> Joseph S. Myers
> josmyers@redhat.com
>