llvm dev - Sep 2021 - [cfe-dev] Intrinsic llvm::isnan

On Tue, Aug 24, 2021 at 1:53 PM Roman Lebedev via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> Regardless of everything, i would like to see a patch that restores
> the -ffast-math handling, and *then* the RFC on what the is-nan check
> should do when -ffast-math is present.
> It is more than possible that the RFC will succeed,
> but i don't think a change like that should happen the way it did.

I find the rationale to be convincing, as to the need for a change. But,
the scope of the proposal is too narrow. We cannot discuss fast-math
semantics changes *only* for "isnan", it needs to be in the context of
the
desired behavior for all operations -- the RFC should cover the entire set
of changes we want to eventually make, even if isnan is the only thing
implemented so far. Discussing this greater scope could result in a
different desired implementation, rather than simply adding
"llvm.isnan"
intrinsic.

Yet, even with that expanded scope, the two halves of the proposal are
still going to be closely linked, so I suspect it still makes sense to
discuss both the strict-fp and fast-math changes in a single RFC.

Anyhow, for the fast-math section, I believe the proposed semantics ought
to be:
  The -ffinite-math-only and -fno-signed-zeros options do not impact the
ability to accurately load, store, copy, or pass or return such values from
general function calls. They also do not impact any of the
"non-computational" and "quiet-computational" IEEE-754
operations, which
includes classification functions (fpclassify, signbit, isinf/isnan/etc),
sign-modification (copysign, fabs, and negation `-(x)`), as well as
the totalorder and totalordermag functions. Those correctly handle NaN,
Inf, and signed zeros even when the flags are in effect. These flags
*do* affect
the behavior of other expressions and math standard-library calls, as well
as comparison operations.

I would not expect this to have an actual negative impact on the
performance benefit of those flags, since the optimization benefits mainly
arise from comparisons and the general computation instructions which are
unchanged.

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949 was mentioned as
> motivational, but i don't see any resolution there,
> it's not even in "confirmed" state.

I agree, this is not at all clear evidence as to GCC's position on the
matter.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210824/6bf95a8f/attachment.html>

>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949 was mentioned as
>> motivational, but i don't see any resolution there,
>> it's not even in "confirmed" state.
>
>
> I agree, this is not at all clear evidence as to GCC's position on the
> matter.
>
Sure, but that demonstrates that they are inclined to such interpretation.

GCC mail list has relevant discussion on this topic:
https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544622.html. They
tried to make the documentation about -ffinite-math-only clearer. The
discussion was based on a view that if -ffinite-math-only was specified, a
floating point value cannot be NaN or Inf. During the discussion an
interesting observation was made:

… double and double under -ffinite-math-only are different types.
Any
> function call from
> one world to the other is dangerous. Inline functions translated in
> different
> TUs compiled with different math flags violate the ODR.

If different parts of a program are compiled with and without
-ffinite-math-only doubles of different flavors can intermix. In the code
compiled with -ffast-math a user cannot check assumption about the value by
calling "assert(!isnan(x));" because `isnan` is replaced with expected
value due to optimizations. The only usable solution in this case could be
UBSAN, which is a much heavier solution.

Two different flavors of double require different mangling. Template
specializations also must be different, in particular, specializations of
`std::numeric_limits<T>` must be different for these two double types, the
methods `has_infinity` and `has_quite_NaN` must return different values.

They agree that it is profitable to treat NaNs and Infs separately. In this
case there would be 4 different double types. It is not clear what to do
with constexpr expressions, should the compiler treat using NaN/Inf as
undefined behavior even if the ultimate result is finite?

Participants agree that such optimizations are not good:

 … the example of simplifying x * 0 to 0 is about preserving
NaNs
> during expression simplification when the FPU would.  I think these
> kind of optimizations are what originally was intended to be allowed
> with -ffinite-math-only - that we started to simplify isnan(x) to false
> was extending the scope and that change wasn't uncontested since
> it makes -ffinite-math-only less useful to some people.

Eventually they came to conclusion:

… if we want a version of
> -ffinite-math-only that's well-behaved in language terms (including
> in terms of the macros that are defined and in the values of
> numeric_limits), perhaps this should be an official (optional) C/C++
> extension that defines what the rules are.

Thanks,
--Serge


On Wed, Aug 25, 2021 at 4:25 AM James Y Knight <jyknight at google.com>
wrote:
> On Tue, Aug 24, 2021 at 1:53 PM Roman Lebedev via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> Regardless of everything, i would like to see a patch that restores
>> the -ffast-math handling, and *then* the RFC on what the is-nan check
>> should do when -ffast-math is present.
>> It is more than possible that the RFC will succeed,
>> but i don't think a change like that should happen the way it did.
>
>
> I find the rationale to be convincing, as to the need for a change. But,
> the scope of the proposal is too narrow. We cannot discuss fast-math
> semantics changes *only* for "isnan", it needs to be in the
context of
> the desired behavior for all operations -- the RFC should cover the entire
> set of changes we want to eventually make, even if isnan is the only thing
> implemented so far. Discussing this greater scope could result in a
> different desired implementation, rather than simply adding
"llvm.isnan"
> intrinsic.
>
> Yet, even with that expanded scope, the two halves of the proposal are
> still going to be closely linked, so I suspect it still makes sense to
> discuss both the strict-fp and fast-math changes in a single RFC.
>
> Anyhow, for the fast-math section, I believe the proposed semantics ought
> to be:
>   The -ffinite-math-only and -fno-signed-zeros options do not impact the
> ability to accurately load, store, copy, or pass or return such values from
> general function calls. They also do not impact any of the
> "non-computational" and "quiet-computational" IEEE-754
operations, which
> includes classification functions (fpclassify, signbit, isinf/isnan/etc),
> sign-modification (copysign, fabs, and negation `-(x)`), as well as
> the totalorder and totalordermag functions. Those correctly handle NaN,
> Inf, and signed zeros even when the flags are in effect. These flags *do*
affect
> the behavior of other expressions and math standard-library calls, as well
> as comparison operations.
>
> I would not expect this to have an actual negative impact on the
> performance benefit of those flags, since the optimization benefits mainly
> arise from comparisons and the general computation instructions which are
> unchanged.
>
>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949 was mentioned as
>> motivational, but i don't see any resolution there,
>> it's not even in "confirmed" state.
>
>
> I agree, this is not at all clear evidence as to GCC's position on the
> matter.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210825/02096364/attachment.html>

I'll take a shot at summarizing where we are. Correct as needed:
1. We want isnan() for strict-FP support. The arguments are similar to why
we added "fneg" as a real instruction.
2. We also need a bunch of other FP classify functions for strict-FP
support to properly deal with SNAN: isinf, isfinite, isnormal, issubnormal,
iszero, fpclassify.
3. There's a 2nd motivation to use at least some of these functions in the
regular LLVM FP environment with fast-math. For isnan(), this boils down to
is "fcmp ord nnan" always true, poison, or unknown? So the root cause
might
really be that we shouldn't have fast-math-flags on fcmp (see
https://bugs.llvm.org/show_bug.cgi?id=38086 and related bugs).
4. The change to the regular FP env requires adding/changing documentation
to the LLVM LangRef and clang (James posted a draft in a reply on 8/24).
5. The change to the regular FP env would make clang/LLVM behave
differently than GCC as noted here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949 . That bug report has
been cited as motivation, but IIUC, the last comment says everything is
behaving as expected and the report should be closed pending a C/C++
language change.

Several people have suggested reverting the original patch, so we can
address this both at larger scale (so we have a clear plan for the other
functions that are needed) and with smaller steps (so we don't break
things).

An argument was made that because the original patch has been in main for a
few weeks, and there are no known bug reports, that it is fine as-is. I
disagree with that: it's not easy to recognize the potential harm (likely
small perf regression), not many users live/test continuously against trunk
for FP fast-math perf, and it's not easy to file bugs against LLVM.



On Wed, Aug 25, 2021 at 7:13 AM Serge Pavlov via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949 was mentioned as
>>> motivational, but i don't see any resolution there,
>>> it's not even in "confirmed" state.
>>
>>
>> I agree, this is not at all clear evidence as to GCC's position on
the
>> matter.
>>
>
> Sure, but that demonstrates that they are inclined to such interpretation.
>
> GCC mail list has relevant discussion on this topic:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544622.html. They
> tried to make the documentation about -ffinite-math-only clearer. The
> discussion was based on a view that if -ffinite-math-only was specified, a
> floating point value cannot be NaN or Inf. During the discussion an
> interesting observation was made:
>
> … double and double under -ffinite-math-only are different types. Any
>> function call from
>> one world to the other is dangerous. Inline functions translated in
>> different
>> TUs compiled with different math flags violate the ODR.
>
>
> If different parts of a program are compiled with and without
> -ffinite-math-only doubles of different flavors can intermix. In the code
> compiled with -ffast-math a user cannot check assumption about the value by
> calling "assert(!isnan(x));" because `isnan` is replaced with
expected
> value due to optimizations. The only usable solution in this case could be
> UBSAN, which is a much heavier solution.
>
> Two different flavors of double require different mangling. Template
> specializations also must be different, in particular, specializations of
> `std::numeric_limits<T>` must be different for these two double
types, the
> methods `has_infinity` and `has_quite_NaN` must return different values.
>
> They agree that it is profitable to treat NaNs and Infs separately. In
> this case there would be 4 different double types. It is not clear what to
> do with constexpr expressions, should the compiler treat using NaN/Inf as
> undefined behavior even if the ultimate result is finite?
>
> Participants agree that such optimizations are not good:
>
>  … the example of simplifying x * 0 to 0 is about preserving NaNs
>> during expression simplification when the FPU would.  I think these
>> kind of optimizations are what originally was intended to be allowed
>> with -ffinite-math-only - that we started to simplify isnan(x) to false
>> was extending the scope and that change wasn't uncontested since
>> it makes -ffinite-math-only less useful to some people.
>
>
> Eventually they came to conclusion:
>
> … if we want a version of
>> -ffinite-math-only that's well-behaved in language terms (including
>> in terms of the macros that are defined and in the values of
>> numeric_limits), perhaps this should be an official (optional) C/C++
>> extension that defines what the rules are.
>
>
> Thanks,
> --Serge
>
>
> On Wed, Aug 25, 2021 at 4:25 AM James Y Knight <jyknight at
google.com>
> wrote:
>
>> On Tue, Aug 24, 2021 at 1:53 PM Roman Lebedev via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>> Regardless of everything, i would like to see a patch that restores
>>> the -ffast-math handling, and *then* the RFC on what the is-nan
check
>>> should do when -ffast-math is present.
>>> It is more than possible that the RFC will succeed,
>>> but i don't think a change like that should happen the way it
did.
>>
>>
>> I find the rationale to be convincing, as to the need for a change.
But,
>> the scope of the proposal is too narrow. We cannot discuss fast-math
>> semantics changes *only* for "isnan", it needs to be in the
context of
>> the desired behavior for all operations -- the RFC should cover the
entire
>> set of changes we want to eventually make, even if isnan is the only
thing
>> implemented so far. Discussing this greater scope could result in a
>> different desired implementation, rather than simply adding
"llvm.isnan"
>> intrinsic.
>>
>> Yet, even with that expanded scope, the two halves of the proposal are
>> still going to be closely linked, so I suspect it still makes sense to
>> discuss both the strict-fp and fast-math changes in a single RFC.
>>
>> Anyhow, for the fast-math section, I believe the proposed semantics
ought
>> to be:
>>   The -ffinite-math-only and -fno-signed-zeros options do not impact
the
>> ability to accurately load, store, copy, or pass or return such values
from
>> general function calls. They also do not impact any of the
>> "non-computational" and "quiet-computational"
IEEE-754 operations, which
>> includes classification functions (fpclassify, signbit,
isinf/isnan/etc),
>> sign-modification (copysign, fabs, and negation `-(x)`), as well as
>> the totalorder and totalordermag functions. Those correctly handle NaN,
>> Inf, and signed zeros even when the flags are in effect. These flags
*do* affect
>> the behavior of other expressions and math standard-library calls, as
well
>> as comparison operations.
>>
>> I would not expect this to have an actual negative impact on the
>> performance benefit of those flags, since the optimization benefits
mainly
>> arise from comparisons and the general computation instructions which
are
>> unchanged.
>>
>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84949 was mentioned as
>>> motivational, but i don't see any resolution there,
>>> it's not even in "confirmed" state.
>>
>>
>> I agree, this is not at all clear evidence as to GCC's position on
the
>> matter.
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210901/6fef389c/attachment.html>