On Thu, May 2, 2024 at 3:48 PM Roger Sayle <ro...@nextmovesoftware.com> wrote:
>
>
> > From: Richard Biener <richard.guent...@gmail.com>
> > 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).
>
> 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 <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.
> > > > >
> > >
> > >
>
