On Thu, May 2, 2024 at 11:34 AM Roger Sayle <ro...@nextmovesoftware.com> wrote:
>
>
> > From: Richard Biener <richard.guent...@gmail.com>
> > On Fri, Apr 26, 2024 at 10:19 AM Roger Sayle <ro...@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 <ro...@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.
> > >
>
>
