RE: [PATCH] PR middle-end/111701: signbit(x*x) vs -fsignaling-nans

["Roger Sayle" <roger@nextmovesoftware.com>]

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