[cfe-dev] [llvm-dev] Should isnan be optimized out in fast-math mode?

[Serge Pavlov via cfe-dev]

Yes, formally this is UB. But removal of `isnan` is also a standard
violation, because `isnan` has definite semantics, which is not preserved
in this case. We already are outside the standard behavior with the `isnan`
removal, so things cannot become worse.

Anyway this is a workaround for emulation only, for the (hypothetical)
case, when performance dropped noticeably due to execution of the checks.
It is now clear if such cases really exist.

Thanks,
--Serge


On Fri, Sep 17, 2021 at 12:56 AM Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Thu, Sep 16, 2021 at 1:31 PM Serge Pavlov via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
> >
> > On Thu, Sep 16, 2021 at 10:02 PM Cranmer, Joshua <
> joshua.cranmer at intel.com> wrote:
> >>
> >> I think you are not adequately summing up the risks of your approach;
> there are three other issues I see.
> >>
> >>
> >>
> >> First, redefining `isnan` as a macro is expressly undefined behavior in
> C (see section 7.1.3, clauses 2 and 3—it’s undefined behavior to define a
> macro as a same name as reserved identifier in a standard library header).
> Conditionally redefining an `isnan` macro is therefore not a permissible
> solution.
> >
> >
> > Defining things like `isnan` is the job of libc, which often is not a
> part of the compiler.
>
> The standard makes no real distinction between what's the job of the
> compiler and what's the job of the library; it's all "the
> implementation" as far as the standard is concerned. FWIW, there are
> plenty of libc things which are produced by the compiler (see
> https://github.com/llvm/llvm-project/tree/main/clang/lib/Headers for
> all the standard library interfaces provided by Clang).
>
> > It does not cause undefined behavior by itself.
>
> A user-defined macro named `isnan` is UB per 7.1.3p1 if the user
> includes <math.h> in the TU.
>
> > Redefining macro defined in system headers may be harmful if the new
> macro is inconsistent with other libc implementation (`errno` comes to
> mind). So this looks like a kind of legal disclaimer.
>
> I would hope so; the standard says explicitly that redefining that
> macro is UB. :-)
>
> ~Aaron
>
> > As with `-ffinite-math-only` the redefinition is used at your own risk.
> For our task to remove the call of pure function the risk is negligible.
> And it is needed only to reproduce the old behavior.
> >
> >>
> >>
> >>
> >> The second thing that has been repeatedly brought up that is missing is
> the fact that `isnan` may still be inconsistently optimized out. `isnan(x)`
> would only be retained in the program if the compiler cannot deduce that
> `x` is the result of a nnan arithmetic operation. If it can deduce that—the
> simplest case being the somewhat questionable `isnan(x + 0)` example, but
> it’s also possible that, e.g., you’re calling `isnan(sum)` on the result of
> a summation, which would be the result of an arithmetic expression
> post-mem2reg/SROA—then the compiler would still elide it. It could be that
> this is less surprising to users than unconditionally optimizing array
> `isnan(x)`, but it should still be admitted that there is a potential for
> surprise here.
> >
> >
> > Regarding your example, this thread already contains consideration of
> this case:
> >
> > On Tue, Sep 14, 2021 at 12:50 AM Serge Pavlov <sepavloff at gmail.com>
> wrote:
> >>
> >> On Mon, Sep 13, 2021 at 11:46 PM Chris Tetreault <ctetreau at quicinc.com>
> wrote:
> >>>
> >>> … is guaranteed to work, and I read that fast-math enables the
> compiler to reason about constructs like `x + 0` being equal to `x`, then
> I’m going to be very confused when:
> >>
> >> You are right, this was a bad idea. Compiler may optimize out `isnan`
> but only when it deduces that the value cannot be NaN, but not due to the
> user's promise. It is especially important for `isinf`. Addition of two
> finite values may produce infinity and there is no universal way to predict
> it. It is probably not an issue for types like float or double, but ML
> cores use halfs or even minifloats, where overflow is much more probable.
> If in the code:
> >> ```
> >> float r = a + b;
> >> if (isinf(r)) {...
> >> ```
> >> `isinf` were optimized out just because -ffinite-math-only is in
> effect, the user cannot check if overflow did not occur.
> >
> >
> > Rules proposed by Richard are also formulated using arguments, not
> results. Now there is no intention to optimize such a case.
> >
> >>
> >>
> >> A final point is that the potential optimization benefits of eliding
> `isnan` are not limited to the cost of running the function itself (which
> are likely to be negligible), but also include the benefits of deleting any
> subsequent code that is attempting to handle NaN values, which may be
> fairly large blocks. A salient example is complex multiplication and
> division, where the actual expansion of  the multiplication and division
> code itself is dwarfed by the recalculation code if the result turns out to
> be a NaN.
> >
> >
> > The code intended for handling NaNs won't be executed in
> -ffinite-math-only mode, if the mode is used correctly. So expenses are
> only the check itself and the associated jump. For the code that does
> intensive calculation they must be negligible. Anyway, the user can
> redefine `isnan`, it is as safe as `-ffinite-math-only` itself.
> >
> >>
> >>
> >> From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Serge
> Pavlov via llvm-dev
> >> Sent: Thursday, September 16, 2021 1:37
> >> To: Chris Tetreault <ctetreau at quicinc.com>
> >> Cc: Arthur O'Dwyer <arthur.j.odwyer at gmail.com>; llvm-dev at lists.llvm.org;
> cfe-dev at lists.llvm.org
> >> Subject: Re: [llvm-dev] [cfe-dev] Should isnan be optimized out in
> fast-math mode?
> >>
> >>
> >>
> >> Let me make some summary. I omit references for brevity, they are
> spread in the thread.
> >>
> >> Treatment of `isnan` with `-ffinite-math-only` has issues:
> >> - There are many users' complaints and disagreement expressed in GCC
> bug tracker and forums about the treatment.
> >> - There are legitimate use cases when `isnan` needs to be called in
> `-ffinite-math-only` mode.
> >> - Users have to invent workarounds to get functionality of `isnan`,
> which results in portability and performance loss.
> >> - There is inconsistency with the behavior of libc, which always does a
> real check, and the compiler, which omits it.
> >> Preserving `isnan` in the code would solve all of them.
> >>
> >> What is the risk?
> >>
> >> `-ffinite-math-only` is an optimization option, so preserving `isnan`
> cannot break the behavior of correct programs. The only possible negative
> impact is some loss of performance. It is unlikely that a real program
> spends so much time in `isnan` calls that it has noticeable effect, but if
> it does, a user can conditionally redefine `isnan` macro.
> >>
> >> Preserving `isnan` in `-ffinite-math-only` mode is safe and makes the
> compiler more reliable and user-friendly.
> >>
> >>
> >>
> >> Thanks,
> >> --Serge
> >
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210917/3a8ef0e1/attachment.html>