Bug 50724 - cmath's floating-point classification implementations inconsistent with math.h
Summary: cmath's floating-point classification implementations inconsistent with math.h
Status: RESOLVED DUPLICATE of bug 25975
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: unknown
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-14 03:37 UTC by Ethan Tira-Thompson
Modified: 2021-09-21 01:14 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2011-10-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ethan Tira-Thompson 2011-10-14 03:37:13 UTC
Given the test code:

    static const float f_nan = 0.f/0.f;
    const int i_nan = *((int*)(&f_nan));
    printf("%g %d %x\n", f_nan, isnan(f_nan), i_nan);

I hope to see:
nan 1 7fc00000

but with the combination of -ffast-math and <cmath>, instead I get:
nan 0 7fc00000

Using just <math.h>, isnan will work. (hence I have filed against libstdc++)  Not surprisingly, adding -fno-finite-math-only can also fix this.

(regarding cmath, both 'isnan' and 'std::isnan' give the same behavior; BTW I'm a little surprised a unscoped 'isnan' compiles without 'using namespace std;'... previous gcc (e.g. Apple 4.2.1) required the 'std'.  I see cmath undef's the isnan macro from math.h, so where is the global isnan coming from?)

Tested with 4.4.3 and 4.5.2.

Ironically, a test with 4.1.2 shows the reverse situation: <math.h> didn't work and <cmath> did work.

I realize the docs for -fno-finite-math-only basically warn "all bets are off" for NaN handling, but as a quality of implementation issue, I'd like to see consistent handling for floating-point classification functions.  A user may not care about strict compliance for operations between non-finite values, but if they are testing for 'bad' values (e.g. isnan), that's probably significant -- otherwise the user wouldn't have added the classification calls to their code in the first place.

Alternatively, a warning would be nice, like "isnan always evaluates to false" when -ffinite-math-only is in effect.
Comment 1 Richard Biener 2011-10-14 09:15:35 UTC
With -ffinite-math-only you are telling that there are no NaNs and thus
GCC optimizes isnan (x) to 0.  That math.h "works" probably means it has
isnan implemented as macro expanding to inline assmbler.
Comment 2 Ethan Tira-Thompson 2011-10-14 14:27:13 UTC
Well, that's not actually true: -fno-finite-math is telling the compiler to assume finite results during computation, but that doesn't mean there are "no NaNs".  For example, I need isnan/isfinite to validate my users' input before I can ensure that invariant afterward.  Clearly the bit pattern still exists in the universe.

I'd also argue classification functions are not "arithmetic" and thus not covered by -fno-finite-math ("Allow optimizations for floating-point arithmetic")

Finally, I think you're being quick to brush off the consistency issue, both with earlier gcc versions and with math.h.  I find it quite disconcerting that isnan behavior changes between the two headers.

Is your concern that __builtin_isnan is called internally during fp computations and we need to optimize that away in order to provide the speed advantages requested by -fno-finite-math?  In that case, I propose cmath should be updated to check __FINITE_MATH_ONLY__ in its user-facing isnan and fall back to a safe method like math.h is currently using (apparently __isnan/__isnanf).

Otherwise, as I established above, I don't think it's safe to 'optimize' away the floating point classification functions: the first issue above is code safety/security, the second is pedantic regarding documentation, and the third is a consistency issue.

Thanks!
Comment 3 Richard Biener 2011-10-14 14:31:33 UTC
(In reply to comment #2)
> Well, that's not actually true: -fno-finite-math is telling the compiler to
> assume finite results during computation

No, it tells the compiler that all inputs are finite.
Comment 4 Jonathan Wakely 2011-10-14 14:47:18 UTC
(In reply to comment #0)
> if they are testing for 'bad' values (e.g. isnan), that's probably significant
> -- otherwise the user wouldn't have added the classification calls to their
> code in the first place.

if they are compiling with -ffinite-math-only (i.e. asserting no NaN arguments or results), that's probably significant -- otherwise the user wouldn't have added the switch to their compilation command in the first place.
Comment 5 Paolo Carlini 2011-10-14 14:56:27 UTC
I haven't really followed this discussion, but I remember a very similar one some time ago, and I suspect that part of the confusion stems from the meaning of "finite-math": maybe some users don't consider calling std::is_nan part of a *mathematical computation* proper, they consider it something preliminary to it, they would like to see *only* arithmetic expressions, transcendental functions calls, etc, optimized per finite-math. I don't know if I have been able to explain myself well enough. Anyway, mine is just a side observation, maybe worth keeping in mind only when writing the documentation.
Comment 6 Paolo Carlini 2011-10-14 15:07:40 UTC
I was also thinking, maybe from the user point of view, a good way to deal with this kind of problem today is splitting the computation in parts via the new optimization attribute and pragma, keep the is_nan & co classifications outside the -fast-math cores.
Comment 7 Michael Matz 2011-10-14 15:36:26 UTC
Hmm.  I do sympathise with Ethan.  I never was very convinced that
__builtin_isnan should return 0 even for NaN bit patterns just because of
-ffast-math.  After all we don't gain much by this in terms of optimization
opportunities (we don't emit calls to it from inside GCC).

And Ethan is right that a developer might indeed first test values coming from
outside the program for fulfilling the guarantees the program is expecting.
After all, -ffinite-math-only is an assertion by the compiler user, but
in light of user input he has to first establish these assertions before
using code paths that need those assertions.  isnan() is precisely the way
to establish them.

And no, I'm not convinced about the argument that the program author simply
shouldn't have used -ffinite-math-only.  He can't control user input, or
better said, the way he can control it is by the classification functions.
Hence we shouldn't remove this (only) way for him to do the classification.

Note how I wrote classification.  IMO the same applies to isnan, isinf,
isnormal, isfinite and fpclassify.

Note further that in this light I don't necessarily think that the precise
testcase from the initial comment as written there should work.  There the
NaN isn't coming from user input but is program generated, and for _those_
values the assertion of -ffinite-math-only should already hold.  Not that
I see any reason to differ between the cases.

Reopening as enhancement request as it would be a change in documented
behaviour.
Comment 9 Ethan Tira-Thompson 2011-10-14 20:07:41 UTC
Thanks, I think Michael hit the nail on the head for summarizing my intention, I'm satisfied to file this as a feature request (although personally I'd still call it a bug ;))

For reference regarding the test case, I agree with your point that 0.f/0 needn't be a reliable NaN generator under -ffast-math.  The original source was actually using std::numeric_limits<float>::quiet_NaN() and/or signaling_NaN(), but I switched to 0.f/0 in order to test under C (not-++) and forgot to switch back.  So yeah, considering numeric_limits still returns the NaN bit patterns regardless of -ff-m-o, it's unbalanced that isnan is prevented from detecting them.  Regardless, disabling classification routines is going beyond the scope of "math" and "arithmetic" as per the name of the flag and its documentation.

(For those oversimplifying -ff-m-o as "No NaNs", perhaps you would like to add a new -fno-nans or perhaps -fno-quiet-nans flag if that is what you really want -ffinite-math-only to mean...)

In the three linked bug reports, the first is basically a "don't do that" without discussion, the second specifically complains about the lack of discussion, and the third is redirected toward fixing an issue in -mno-ieee without resolving the -ffinite-math-only aspect.
Comment 10 Marc Glisse 2011-10-14 21:12:40 UTC
As a library writer, having isnan return false is precisely what I am expecting from -ffinite-math-only. In my code, I implement regular computations for finite numbers, and I need some special cases for infinite and nan values, which I test with isnan and isfinite. With -ffinite-math-only, the compiler is able to remove those special cases entirely.
Comment 11 Ethan Tira-Thompson 2011-10-14 22:07:44 UTC
Marc: is this code perusable? I'm curious because I expect either the calculations may generate NaN or not at all.  If they might and you even have test cases to handle it, then I'm surprised you would ever want to support running with -ff-m-o.  Conversely if you knew the code doesn't generate the nonfinite values, then you don't need the classifications in the first place...?

I'm guessing (and apologies if this is inaccurate) that this might boil down to saying that you want to interpret an end user setting -ff-m-o as an opportunity to skip validating their input or skip doing assertions during its processing, which could be a reasonable thing to do, but that's a choice I'd rather leave to individual developers, e.g. can also wrap code with #if __FINITE_MATH_ONLY__==0 or such...

Or in other words, it's only a missed optimization if you wind up with classification calls, whereas it's a full-fledged execution error when NaN gets past validation.
Comment 12 Richard Biener 2011-10-15 08:50:56 UTC
(In reply to comment #11)
> Marc: is this code perusable? I'm curious because I expect either the
> calculations may generate NaN or not at all.  If they might and you even have
> test cases to handle it, then I'm surprised you would ever want to support
> running with -ff-m-o.  Conversely if you knew the code doesn't generate the
> nonfinite values, then you don't need the classifications in the first
> place...?
> 
> I'm guessing (and apologies if this is inaccurate) that this might boil down to
> saying that you want to interpret an end user setting -ff-m-o as an opportunity
> to skip validating their input or skip doing assertions during its processing,
> which could be a reasonable thing to do, but that's a choice I'd rather leave
> to individual developers, e.g. can also wrap code with #if
> __FINITE_MATH_ONLY__==0 or such...
> 
> Or in other words, it's only a missed optimization if you wind up with
> classification calls, whereas it's a full-fledged execution error when NaN gets
> past validation.

You can switch between explicit checking and trapping for example, by
switching between -ffinite-math-only and -fno-trapping-math.

Note that in general it is impossible to decide whether an argument of
isnan() is from user input or previous computation.  Which means that
making isnan() special for -ffinite-math-only makes as much sense as
special-casing any math library function (that may also take user input).

This has been discussed to death already, and the present behavior is how
GCC behaved since ever.  It's not a bug.  The documentation states

"Allow optimizations for floating-point arithmetic that assume
that arguments and results are not NaNs or +-Infs."

it is clear that isnan(x) may then be optimized assuming that x is
not NaN.
Comment 13 Marc Glisse 2011-10-15 09:05:57 UTC
(In reply to comment #11)
> I'm guessing (and apologies if this is inaccurate) that this might boil down to
> saying that you want to interpret an end user setting -ff-m-o as an opportunity
> to skip validating their input or skip doing assertions during its processing,

Yes, exactly.

> which could be a reasonable thing to do, but that's a choice I'd rather leave
> to individual developers, e.g. can also wrap code with #if
> __FINITE_MATH_ONLY__==0 or such...

Not very portable afaik.

> Or in other words, it's only a missed optimization if you wind up with
> classification calls,

-ffinite-math-only exists for the sole purpose of allowing extra optimizations at the expanse of correctness if the contract is broken.

> whereas it's a full-fledged execution error when NaN gets
> past validation.

A full-fledged user error to have used this flag ;-)

Actually, I can understand why you'ld want to be able to write assert(!isnan(x)) and get a useful result. If on the other hand you want if(isnan(x))f();else g(); then g should be in a separate translation unit that can use -ffinite-math-only (and using LTO may require a lot of prayers in that case) or it could use the optimize attribute/pragma. This could work for the assertion as well: have a validate_input function compiled without the flag.

I am not sure if it is possible to have it both ways: be able to validate the input and still be able to optimize headers.
Comment 14 Ethan Tira-Thompson 2011-10-15 14:01:11 UTC
Richard said:
> The documentation states
> "Allow optimizations for floating-point arithmetic that
> assume that arguments and results are not NaNs or +-Infs."

Yes, that's my argument as well.  Note ARITHMETIC.  Also note MATH in the name of both --finite-MATH-only, and -ffast-MATH it falls under.  And it doesn't reference the 'math library' to qualify that.

Basically, if you want to close on this point, I want to see you explicitly argue why the classification functions should be considered arithmetic.

I'm going to nail this down and list there are 5 classification functions (fpclassify, infinite, isinf, isnan, isnormal) and the vast majority of the other library functions are obviously proper arithmetic operations.  The ones that aren't (signbit, copysign, nextafter, nan), you're exactly right that we should carefully consider the result of NaN optimization ("special cases").  You don't have to do this if *you* don't want to, but it should be done and it sounds like no one has.

> This has been discussed to death already, and the present behavior is how
> GCC behaved since ever.

Except also NOT TRUE.  It currently doesn't behave this way with math.h.  It didn't behave this way in 4.1 (Fedora) or 4.2 (Apple).  I only got screwed by this by the CHANGE in behavior on upgrading to 4.4.  (Not sure about 4.3).  I already presented this in the original post at the top (except the Apple test is a new data point; FYI Apple gcc math.h also 'works', so either 4.2 was generally consistent or Apple likes to patch theirs for consistency)

This is further evidence gcc has not had a good policy discussion of where NaN optimizations should be applied, because the implementation keeps changing, and it's not even consistent between math.h and cmath within any given version other than 4.2-Apple.

Marc said:
>> __FINITE_MATH_ONLY__
> Not very portable afaik.

Neither is -ffast-math, add a 'defined(__FINITE_MATH_ONLY__) &&' and it will be applied opportunistically when available, or even better: the user can -D__FINITE_MATH_ONLY__=1 themselves on non-gcc platforms and still get the performance benefit you're looking for even without -ffast-math support, so it's a double win.  IMHO, on the other hand it's harder and more error prone to override isnan with my own implementation.
Comment 15 Andrew Pinski 2011-10-15 18:00:10 UTC
It has always been assumed that arithmetic includes isnan.  isnan can be implemented as a macro defining to !(a==a) which then becomes an arithmetic.
Comment 16 Richard Biener 2011-10-16 09:59:35 UTC
(In reply to comment #14)
> Richard said:
> > The documentation states
> > "Allow optimizations for floating-point arithmetic that
> > assume that arguments and results are not NaNs or +-Infs."
> 
> Yes, that's my argument as well.  Note ARITHMETIC.  Also note MATH in the name
> of both --finite-MATH-only, and -ffast-MATH it falls under.  And it doesn't
> reference the 'math library' to qualify that.
> 
> Basically, if you want to close on this point, I want to see you explicitly
> argue why the classification functions should be considered arithmetic.
> 
> I'm going to nail this down and list there are 5 classification functions
> (fpclassify, infinite, isinf, isnan, isnormal) and the vast majority of the
> other library functions are obviously proper arithmetic operations.  The ones
> that aren't (signbit, copysign, nextafter, nan), you're exactly right that we
> should carefully consider the result of NaN optimization ("special cases"). 
> You don't have to do this if *you* don't want to, but it should be done and it
> sounds like no one has.
> 
> > This has been discussed to death already, and the present behavior is how
> > GCC behaved since ever.
> 
> Except also NOT TRUE.  It currently doesn't behave this way with math.h.

math.h is not part of GCC, if math.h implements isnan in a way that skips
GCC (by making it use inline assembler) then that doesn't show GCC does
not perform the optimization (you can check with using __builtin_isnan
instead of isnan).

The only way out I see that not breaks other users uses would be a new
flag, like -fpreserve-ieee-fp-classification that, ontop of -ffinite-math-only,
would disable the optimization for the classification functions.
(Note that they are folded to arithmetic, !(x==x), so that transform has
to be disabled as well, and on some architectures you might get library
calls because of this instead of inline expansions).
Comment 17 Ethan Tira-Thompson 2011-10-17 04:12:31 UTC
Richard said:
> math.h is not part of GCC

But the point is there is value in consistency between math.h and cmath regardless of who owns math.h.  I'm asserting that this value is greater than that gained by 'optimizing away' the classification functions in cmath.  Inconsistency leads to confused users and therefore bugs, missed optimization is only going to cause slower code.  I get that you want to make the most of -ffast-math, and if it were a big speedup it could be worthwhile, but it seems reasonable that if someone is serious about optimizing away their classification sanity checks in a release build, they would be better served by using assert() or an #ifdef instead of relying of the vagaries of -ffast-math for this purpose.

> The only way out I see that not breaks other users uses would be a new
> flag, like -fpreserve-ieee-fp-classification that, ontop of -ffinite-math-only,

I'm not opposed to a new flag, but I'd suggest the reverse semantics.  Disabling classification is an extra degree of non-compliance beyond ignoring non-finite math operations.  I'd rather users add flags to progressively become less compliant, rather than add a flag to get some compliance back.

But to rewind a second, I want to address the "breaks other users" comment... here is the status AFAIK:
1) Older versions (4.1, 4.2) of gcc did not do this optimization of classification functions.  Thus, "legacy" code expects classification to work even in the face of -ffast-math, which was changed circa 4.3/4.4
2) Removing classification 'breaks' code because it fundamentally strips execution paths which may otherwise be used.
3) Leaving classification in could be considered a missed optimization, but is at worst only a performance penalty, not a change in execution values.
4) Personal conjecture: I doubt the classification routines are a performance bottleneck in the areas where -ff-m-o is being applied, so (3) is pretty minimal.  And I seriously doubt anyone is relying on the removal of classification in a code-correctness context, that doesn't make any sense.

Are we on the same page with these points?  So if you are concerned with the breakage of existing code, isn't the solution to revert to the previous scope of the -ff-m-o optimization ASAP, and then if there is a desire to extend the finite-only optimization to classification functions, *that* would be a new feature request, perhaps with its own flag?

> (Note that they are folded to arithmetic, !(x==x), so that transform
> has to be disabled as well, and on some architectures you might get
> library calls because of this instead of inline expansions).

I'd rather leave comparison optimizations as they are under -ff-m-o, that seems a sensible expectation of the 'arithmetic' scope, and is relatively well-known, long-standing effect of -ffast-math.  It's only the 5 explicit fp classification calls which I think deserve protection to allow data validation in a non-hacky manner before doing core computations with the finite invariant.

Unless you are saying things like std::isnan cannot be implemented separately from !(x==x)?  That has not been my understanding.
Comment 18 Richard Biener 2011-10-17 07:12:38 UTC
(In reply to comment #17)
> Richard said:
> > math.h is not part of GCC
> 
> But the point is there is value in consistency between math.h and cmath
> regardless of who owns math.h.  I'm asserting that this value is greater than
> that gained by 'optimizing away' the classification functions in cmath. 
> Inconsistency leads to confused users and therefore bugs, missed optimization
> is only going to cause slower code.

You get the same inconsistency if math.h implements isnan as

#define isnan(x) (!((x)==(x)))

which is a valid implementation.  That would be optimized with your
suggested -ffinite-math-only implementation, but not when the library
implements isnan as

#define isnan(x) __builtin_isnan(x)

So what's your point again?

> I get that you want to make the most of
> -ffast-math, and if it were a big speedup it could be worthwhile, but it seems
> reasonable that if someone is serious about optimizing away their
> classification sanity checks in a release build, they would be better served by
> using assert() or an #ifdef instead of relying of the vagaries of -ffast-math
> for this purpose.
> 
> > The only way out I see that not breaks other users uses would be a new
> > flag, like -fpreserve-ieee-fp-classification that, ontop of -ffinite-math-only,
> 
> I'm not opposed to a new flag, but I'd suggest the reverse semantics. 
> Disabling classification is an extra degree of non-compliance beyond ignoring
> non-finite math operations.  I'd rather users add flags to progressively become
> less compliant, rather than add a flag to get some compliance back.

The point is backward-compatible behavior of -ffast-math.  We can't and
should not break this without a very very very good reason.  Which this isn't.

> But to rewind a second, I want to address the "breaks other users" comment...
> here is the status AFAIK:
> 1) Older versions (4.1, 4.2) of gcc did not do this optimization of
> classification functions.  Thus, "legacy" code expects classification to work
> even in the face of -ffast-math, which was changed circa 4.3/4.4

Sure they did.

> 2) Removing classification 'breaks' code because it fundamentally strips
> execution paths which may otherwise be used.
> 3) Leaving classification in could be considered a missed optimization, but is
> at worst only a performance penalty, not a change in execution values.
> 4) Personal conjecture: I doubt the classification routines are a performance
> bottleneck in the areas where -ff-m-o is being applied, so (3) is pretty
> minimal.  And I seriously doubt anyone is relying on the removal of
> classification in a code-correctness context, that doesn't make any sense.

I have written code that you can switch between using FP exceptions and
explicit checks at certain points.  I expect the checks to be optimized
away when using the FP exception path.

> Are we on the same page with these points?  So if you are concerned with the
> breakage of existing code, isn't the solution to revert to the previous scope
> of the -ff-m-o optimization ASAP, and then if there is a desire to extend the
> finite-only optimization to classification functions, *that* would be a new
> feature request, perhaps with its own flag?
> 
> > (Note that they are folded to arithmetic, !(x==x), so that transform
> > has to be disabled as well, and on some architectures you might get
> > library calls because of this instead of inline expansions).
> 
> I'd rather leave comparison optimizations as they are under -ff-m-o, that seems
> a sensible expectation of the 'arithmetic' scope, and is relatively well-known,
> long-standing effect of -ffast-math.  It's only the 5 explicit fp
> classification calls which I think deserve protection to allow data validation
> in a non-hacky manner before doing core computations with the finite invariant.
> 
> Unless you are saying things like std::isnan cannot be implemented separately
> from !(x==x)?  That has not been my understanding.

No, I said that GCC itself unconditionally transforms isnan to !(x == x)
(independent of -ffinite-math-only).

If you really want to go forward you have to produce a patch, test it and
post it to gcc-patches@gcc.gnu.org.

*** This bug has been marked as a duplicate of bug 25975 ***
Comment 19 Ethan Tira-Thompson 2011-10-17 15:31:12 UTC
Richard said:
>> 1) Older versions (4.1, 4.2) of gcc did not do this optimization
> Sure they did.

Dude, I tested this in my very first post.  I'm only here because we had working code which has broken on the upgrade to Ubuntu 10.04.  There's no point in discussing with you if you're just going to deny the state of the world and not offer any data to back up your side.  But before you start coding a test case, read on, turns out I've already done this for you below.

> I expect the checks to be optimized away when using the FP exception path.

I hate using this rationale, but have you considered that this is not a required or portable behavior and you shouldn't rely on it?  And what happens if the checks are left in?  Is anything actually 'broken' by this?  You keep using that word, I do not think it means what you think it means.  I lose data validation when the classifications are disabled.  My code does something fundamentally different as a result.  This is demonstrable 'breakage'.  To quote a wise man, "We should not break this without a very very very good reason.  Which this isn't." ;)

> [isnan implementation ...] So what's your point again?

There are various ways to define isnan and friends.  I'm requesting one which is consistent with math.h's isnan and also previous behavior.  That could be bitmask operations, it could be calling __isnan or math.h's isnan(), etc. 

I think we need some new data to move this along...
I did a little investigation on how these functions have been defined over time:

In 4.1 and 4.2, cmath provides:
  std::isnan() forwards to __gnu_cxx::__capture_isnan()
  __gnu_cxx::__capture_isnan() forwards to math.h ::isnan()
  math.h ::isnan forwards to __isnan()
as of this patch:
http://gcc.gnu.org/viewcvs/trunk/libstdc%2B%2B-v3/include/c_std/cmath?r1=119611&r2=130443
This has changed to simply:
  std::isnan() forwards to __builtin_isnan()

Huh, that's interesting, let's cut out the middle men and see what these underlying functions do across history:
  const float qNaN = std::numeric_limits<float>::quiet_NaN();
  std::cout << __builtin_isnan(qNaN) << ' '
            << __isnan(qNaN) << ' '
            << !(qNaN==qNaN) << '\n';

Compiled with -ffast-math on 3 different machines
0 1 0 // Fedora 9, gcc 4.1.2
0 1 0 // Apple 10.7, gcc 4.2.1
0 1 0 // Ubuntu 10.04, gcc 4.4.3

Hey, you're right insofar as the 'internal' implementations haven't changed at all.  What changed is where cmath sends its implementation (and fyi, I did originally file this under libstdc++, just by lucky guess ;).  cmath used to explicitly call the math.h definition (aka __isnan), which is not optimized by -ffast-math.

For reference, I checked the headers on the Apple machine as well, and found some relevant results.  cmath starts the same with std::isnan → __capture_isnan → ::isnan, but the abridged math.h ::isnan definition is:
  #if defined( __GNUC__ ) && 0 == __FINITE_MATH_ONLY__ 
    #define isnan(x) __inline_isnanT((T)(x))
    inline int __inline_isnanT( T __x ) { return __x != __x; }
  #else
    #define isnan(x) __isnanT((T)(x))
    extern int __isnanT(T);
  #endif
(full source http://www.opensource.apple.com/source/Libm/Libm-315/Source/Intel/math.h)

Which is all prepended by this comment:
> Yes, that's right. You only get the fast iswhatever() macros if you do NOT
> turn on -ffast-math.  These inline functions require the compiler to be
> compiling to standard in order to work. -ffast-math, among other things,
> implies that NaNs don't happen. The compiler can in that case optimize
> x != x to be false always, wheras it would be true for NaNs. That breaks
> __inline_isnan() below.

So, to whatever degree you care what major users are doing, at least one popular platform considers it breakage to disable fp classification, and falls back on a function call to preserve it in the face of -ffast-math.

> The point is backward-compatible behavior of -ffast-math.

I agree!  And I even found the exact patch that broke it.  So would anyone (Hi Paolo :)) like to chime in on the rationale of the linked patch above and how best to restore consistency of the user-facing classification functions?

In particular, can std::isnan (and its classification friends) be redirected back to their math.h implementation?

Or if there's a performance hit with using math.h, could they be wrapped in an #ifdef similar to what Apple did, so that you use __builtin_isnan normally, but capture to math.h when __FINITE_MATH_ONLY__ is detected?

Thanks!
Comment 20 Paolo Carlini 2011-10-17 15:40:52 UTC
No way the library is going to do anything else but forward to the builtins here.
Comment 21 Paolo Carlini 2011-10-17 15:43:57 UTC
.

*** This bug has been marked as a duplicate of bug 25975 ***
Comment 22 Ethan Tira-Thompson 2011-10-17 16:46:27 UTC
Paolo: thanks for the quick reply, but it would help if you could explain why that is the case?  Also, a follow-up, is __builtin_isnan and friends used anywhere except cmath?  It appears not, other than tr1/special_function_util.h, which is also providing an __isnan.
Comment 23 Paolo Carlini 2011-10-17 16:49:03 UTC
.

*** This bug has been marked as a duplicate of bug 25975 ***
Comment 24 Ethan Tira-Thompson 2011-10-17 18:38:05 UTC
Please don't change bug status without comment.  We already have bug 25975 listed as a duplicate in the history several times now.  That bug report was never really resolved, the user just found a workaround and went away.  Further, he was using math.h, which has been stated is outside gcc, whereas I am addressing the cmath implementation.  So not even the same issue anyway.

So that we can find a resolution to this issue, please explain why cmath cannot use math.h?  I'm assuming there's a good reason, I just don't know the history there.  Is it performance, portability, licensing, what? (Those who don't understand history (me) are doomed to repeat it...)

Alternatively, another potential solution would be to tweak the __builtins to restore the original behavior in cmath, which is what you (Paolo) were actually suggesting early on... grepping around, it certainly looks like cmath is the only thing using them.  Am I missing anything there?
Comment 25 Paolo Carlini 2011-10-17 18:53:56 UTC
.

*** This bug has been marked as a duplicate of bug 25975 ***
Comment 26 Ethan Tira-Thompson 2011-10-17 19:21:56 UTC
I'm just asking about a patch that broke existing code and this is how you respond?  It's nothing personal, mistakes happen, I'm just trying to figure out how to fix it for others.  (And why'd you remove someone else's name off the cc list?  Whatever...)  If you don't have time to support your code then just say so or ignore it, no need to be an ass about it.
Comment 27 Andrew Pinski 2011-10-17 19:25:06 UTC
The patch just made cmath use the builtins and the builtin isnan is implemented as !(a==a) which gets optimized to false because with only finite math, a has to equal itself.

*** This bug has been marked as a duplicate of bug 25975 ***
Comment 28 Ethan Tira-Thompson 2011-10-17 20:21:27 UTC
So then there are a variety of potential solutions to evaluate:

A) Don't use the builtin, go back to __isnan, perhaps only when -ff-m-o is in effect.  Paolo says this is bad, but it's not clear why.  Seems like a decent solution except for his apparent disapproval.

B) Change the builtin implementation, just for example with bitmasks:
  inline bool isnan(float x) {
    const int EXP  = 0x7f800000;
    const int FRAC = 0x007fffff;
    const int y = *((int*)(&x));
    return ((y&EXP)==EXP && (y&FRAC)!=0);
  }
(this is what I'm using to reproduce isnan in my own code, maybe there are better ways?)
But this may be a portability issue to support different float point standards, right?  That's just as much an argument to not have users trying to do this themselves though.

C) This is getting out of my knowledge, but Paolo also suggested "splitting the computation in parts via the new optimization attribute and pragma, keep the is_nan & co classifications outside the -fast-math cores."  That sounds elegant and possibly straightforward if it's just a matter of adding an attribute.

D) Combination of A, B and/or C: add new 'safe' builtins, have cmath use them when -ff-m-o is in effect, but otherwise use the presumably faster/more easily optimized normal builtins when these optimizations won't change behavior.

E) Screw compatibility with older gcc or Apple's current gcc or other forks, just update the documentation.  Make it clear -ff-m-o includes both classification and arithmetic functions, that this behavior is not portable or consistent even within math.h vs. cmath, and let the users suck it up.  Generating warnings on calling the nan-generation functions like nan(char*) or numeric_limits::quiet_NaN() and maybe the classification functions too (e.g. "isX always evaluates to true/false") would be a plus.

I'll be pretty disappointed with gcc quality control if you really choose (E), but it's there.
Comment 29 Jonathan Wakely 2011-10-17 21:05:02 UTC
I only see one person being an ass here. I'll update the documentation to make it clear, just stop reopening this.

*** This bug has been marked as a duplicate of bug 25975 ***
Comment 30 Jonathan Wakely 2011-10-17 21:21:45 UTC
(In reply to comment #28)
> So then there are a variety of potential solutions to evaluate:
> 
> A) Don't use the builtin, go back to __isnan, perhaps only when -ff-m-o is in
> effect.  Paolo says this is bad, but it's not clear why.  Seems like a decent
> solution except for his apparent disapproval.

Maybe you should consider that the maintainers have actually thought about it and might know what they're talking about?  What is __isnan? Is that reliably available on all supported platforms?  (Hint: no)  Is __builtin_isnan reliably available? (Hint: yes)

Being patronising because you don't understand all the issues won't win you any friends.

> B) Change the builtin implementation, just for example with bitmasks:
>   inline bool isnan(float x) {
>     const int EXP  = 0x7f800000;
>     const int FRAC = 0x007fffff;
>     const int y = *((int*)(&x));
>     return ((y&EXP)==EXP && (y&FRAC)!=0);
>   }
> (this is what I'm using to reproduce isnan in my own code, maybe there are
> better ways?)
> But this may be a portability issue to support different float point standards,
> right?  That's just as much an argument to not have users trying to do this
> themselves though.

If you want your code to conform to floating point standards THEN DON'T USE -ffinite-math-only! It's there in the manual for all to see.

> C) This is getting out of my knowledge, but Paolo also suggested "splitting the
> computation in parts via the new optimization attribute and pragma, keep the
> is_nan & co classifications outside the -fast-math cores."  That sounds elegant
> and possibly straightforward if it's just a matter of adding an attribute.

That would be something for user code to do, not the library.
Feel free to do it in your code.
 
> D) Combination of A, B and/or C: add new 'safe' builtins, have cmath use them
> when -ff-m-o is in effect, but otherwise use the presumably faster/more easily
> optimized normal builtins when these optimizations won't change behavior.
> 
> E) Screw compatibility with older gcc or Apple's current gcc or other forks,
> just update the documentation.  Make it clear -ff-m-o includes both
> classification and arithmetic functions, that this behavior is not portable or
> consistent even within math.h vs. cmath, and let the users suck it up. 
> Generating warnings on calling the nan-generation functions like nan(char*) or
> numeric_limits::quiet_NaN() and maybe the classification functions too (e.g.
> "isX always evaluates to true/false") would be a plus.

Feel free to open a separate PR requesting a warning.
 
> I'll be pretty disappointed with gcc quality control if you really choose (E),
> but it's there.

I think we can live with your disappointment.

You are using a feature which is documented to alter behaviour for code using NaNs, it clearly says "it can result in incorrect output for programs which depend on an exact implementation of IEEE or ISO rules/specifications for math functions".  If you don't like it, don't use the option.
Comment 31 Ethan Tira-Thompson 2011-10-18 00:33:41 UTC
I don't see what the hurry is to close the bug while it's still under discussion.  I guess you guys just like hit that 'resolved' button before you've actually committed any resolution.  OK, when in Rome...

> Maybe you should consider that the maintainers have actually thought about
> it and might know what they're talking about?  What is __isnan? Is that
> reliably available on all supported platforms?  (Hint: no)  Is __builtin_isnan
> reliably available? (Hint: yes)

This was exactly the information I was asking for.  I explicitly asked if it was a portability issue (vs. performance or licensing, thought perhaps maybe it was some GPL thing).  Maybe it sounded patronizing because it was such a obvious question to you, but please have a little patience with people who are new to the project internals.  Heck, how many people do you think even know that __isnan is from libm and __builtin_isnan is from gcc, or the relationship between the two?  This stuff is not well documented nor common knowledge, and if maintainers don't want to answer questions then what do you expect?

A fair bit of this thread has been wasted on a subset of maintainers saying 'gcc hasn't changed, finite-math has always been this way', directly in response to me showing code demonstrating the change and eventually tracking down the patch that caused it in spite of the derogatory comments, like even yours just now.  So similarly you might consider a user who has spent a fair bit of time finding the problem and presenting it to you on a silver platter, is probably not hallucinating the whole thing. (Hint: no)  Fair enough?

This is probably moot, but if you'd like to get this back on topic, I'm still on board.

So then, just for reference, does no part of gcc rely on libm now?  Or is there a config mechanism that lets parts of gcc use libm when it is available?  I ask because gcc (well, libstdc++) used to use libm, so that begs the question what is the current status?

> If you want your code to conform to floating point standards THEN DON'T
> USE -ffinite-math-only! It's there in the manual for all to see.

My concern is not 'conformance' per se.  Obviously then I just wouldn't be using the flag.

1) Inconsistency between math.h and cmath (unpredictable optimization application, maybe I use math.h, but someone includes cmath before my header, perhaps a precompiled header across translation units, now my code behaves differently, and this might vary per-translation unit within the program.)
2) Inconsistency with older versions/forks of gcc (breaking legacy code/portability, maybe even security issues?)
3) Insufficient documentation regarding the expected degree of non-compliance ("arithmetic" includes classification?)
4) What is really the *desired* degree of non-compliance in practical usage, is this throwing out the baby with the bathwater?  (For most of the finite-math effects, garbage-in-garbage-out is fair enough; but the classification functions often lead to code paths with side effects, e.g. triggering UI, skipping data records, etc, so it's a much broader effect.)

So yes, I could just not use the flag.  But it's useful, it's just gotten too aggressive.  As a user, it's hard to reproduce isnan (and friends) to do validation when finite-math is active (and since build systems make it easy for end users to recompile with additional flags, it's not entirely controllable either, other than saying "don't do that", I'd rather it wasn't all-or-nothing.)  Since gcc used to provide this more limited finite-math optimization, I was hoping it would not be hard to restore it.  I also used to think there would be a concern regarding how the user-visible scope of finite-math had changed, breaking code. *shrug*

> That would be something for user code to do, not the library.
> Feel free to do it in your code.

I can't add an attribute to the system isnan from my user code, or can I?  I've never been quite sure what Paolo was referring to, can someone clue me in?  It sounded like he was saying the builtins could be given an attribute to specify -fno-finite-math-only which would only affect their specific usage?  Maybe he just meant I could apply different flags in different translation units of my own code, in which case I've misunderstood.  Basically, what's the "new optimization attribute and pragma"?

Thanks!
Comment 32 Michael Matz 2011-10-18 01:33:10 UTC
To be honest, this bug report is not under any discussion anymore.  I tried to
get any sort of sanity, but in the end it's all about egos; you won't
get what you want, it's really useless to reopen this particular report.
The libstdc++ maintainers aren't as sensible as the Apple maintainers, the
middle-end maintainers aren't as useful as they should be (hiding behind less
than useful "but that's how it's documented" arguments).  And just generally the
responses to understandable requests are more than lacking in defense of not
implementing it.

(I think it's hopeless to include even more justifications for your use cases,
it'll just be food for the reverse-trolls)

He's not a troll.  The bug-closers _are_, and the answers to closing are
severely lacking in any sort of justification.  And yes, I'm aware of past
discussions.  And no, I don't think any of it has much merit for the discussion
at hand.  They are either exaggerating (but, OMG, what to do with the
RTL "<>" -> "!=" transformation???) (answer, "nothing"), and "it's all quite difficult", or "but currently we do builtin_isnan to 'x!=x', and we can't
recover from that" or they're not capturing the problem at all.  Many of the
justifications even just turn back to "but that's how it's documented since
<whenever>", and that's even more sorry than any other sort of "justification".
Just because a past mistake was documented as such doesn't mean it's right.

So, reporter, please go away, GCC won't fix your problem, even though it would
be reasonable to change.  Instead I'd advise you to some other more reasonable
compiler, LLVM, or maybe a commercial one.  GCC is all about defending past
mistakes in order not to change anything, not at all about being helpful to
the user.

From the proponents of the current state of affairs I would appreciate at least
a hint of where this was "discussed to death" already, the three PRs from
Andrew certainly are not a case.  I'd speculate most of them were argued from
the point of "let's not change anything but rather argue that current behaviour
is the right one (because that's less work, although I won't say that)".
Comment 33 Jonathan Wakely 2011-10-18 09:41:50 UTC
(In reply to comment #31)
> I can't add an attribute to the system isnan from my user code, or can I?  I've
> never been quite sure what Paolo was referring to, can someone clue me in?  It
> sounded like he was saying the builtins could be given an attribute to specify
> -fno-finite-math-only which would only affect their specific usage?  Maybe he
> just meant I could apply different flags in different translation units of my
> own code, in which case I've misunderstood.  Basically, what's the "new
> optimization attribute and pragma"?

Read the frabjous manual:
http://gcc.gnu.org/onlinedocs/gcc/Function-Specific-Option-Pragmas.html
http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007boptimize_007d-function-attribute-2496

You shouldn't need to put code in separate translation units, you can specify different optimizations per function (though I haven't checked if that works for -fno-finite-math-only)
Comment 34 Richard Biener 2011-10-18 10:03:53 UTC
(In reply to comment #32)
> To be honest, this bug report is not under any discussion anymore.  I tried to
> get any sort of sanity, but in the end it's all about egos; you won't
> get what you want, it's really useless to reopen this particular report.
> The libstdc++ maintainers aren't as sensible as the Apple maintainers, the
> middle-end maintainers aren't as useful as they should be (hiding behind less
> than useful "but that's how it's documented" arguments).  And just generally
> the
> responses to understandable requests are more than lacking in defense of not
> implementing it.
> 
> (I think it's hopeless to include even more justifications for your use cases,
> it'll just be food for the reverse-trolls)
> 
> He's not a troll.  The bug-closers _are_, and the answers to closing are
> severely lacking in any sort of justification.  And yes, I'm aware of past
> discussions.  And no, I don't think any of it has much merit for the discussion
> at hand.  They are either exaggerating (but, OMG, what to do with the
> RTL "<>" -> "!=" transformation???) (answer, "nothing"), and "it's all quite
> difficult", or "but currently we do builtin_isnan to 'x!=x', and we can't
> recover from that" or they're not capturing the problem at all.  Many of the
> justifications even just turn back to "but that's how it's documented since
> <whenever>", and that's even more sorry than any other sort of "justification".
> Just because a past mistake was documented as such doesn't mean it's right.
> 
> So, reporter, please go away, GCC won't fix your problem, even though it would
> be reasonable to change.  Instead I'd advise you to some other more reasonable
> compiler, LLVM, or maybe a commercial one.  GCC is all about defending past
> mistakes in order not to change anything, not at all about being helpful to
> the user.
> 
> From the proponents of the current state of affairs I would appreciate at least
> a hint of where this was "discussed to death" already, the three PRs from
> Andrew certainly are not a case.  I'd speculate most of them were argued from
> the point of "let's not change anything but rather argue that current behaviour
> is the right one (because that's less work, although I won't say that)".

Feel free to provide a patch that splits -ffinite-math-only into two pieces,
excempting optimizing the classification builtins from -ffast-math.  Please
make sure that with that code generation quality is not worse comparing
the new -ffast-math to the old -ffast-math -fno-finite-math-only (thus
make sure the classification macros are still expanded inline).  For that
to work you probably have to change RTL optimizers to not do comparison
code folding based on flag_finite_math_only (so you can expand isnan
to !(x==x) even with -ffinite-math-only).  Tree optimizers should have
already converted all these codes (so you still need to make sure to
not fold the classification builtins to comparisons at the tree level).

I would approve a patch along the above lines but I won't spend any time
on producing such patch.

I will also approve a patch that amends the current documentation
of -ffinite-math-only to say that it includes optimizing FP classification
macros.
Comment 35 Ethan Tira-Thompson 2011-10-18 21:09:07 UTC
Thanks all for the info!

I should have realized there was literally an attribute/pragma called 'optimize' (duh), and it's already in the docs... for some reason I had gotten the impression this was a brand new development (i.e. hadn't been released yet), I should've checked.

FYI, the optimize attribute seems to work for fast-math, but interestingly, the pragma doesn't.  I've created a new bug 50782 for this.

> change RTL optimizers to not do comparison code folding based on
> flag_finite_math_only (so you can expand isnan to !(x==x) even with
> -ffinite-math-only)

What would you say to a solution which allows finite math to optimize the comparisons, but at the cost of explicit classification being full function calls?  And of course when finite math is not in effect, everything is inlined as normal.

This would allow core computations to be fully optimized and only explicit classification calls would be affected.  This presumes the classification calls are less common in order to be a good tradeoff, my intuition is that this is the case.  It also allows those of you who want to optimize-away nan checks to continue to do so, just use a (x!=x), and as a bonus this approach will also work consistently between gcc variants.  What do you think?

This is also easy to implement without touching the compiler source, just apply attributes in libstdc++ to keep the classification calls no-finite-math-only, for example the isnan implementation would be:

#if defined(__FINITE_MATH_ONLY__) && __FINITE_MATH_ONLY__
    // apply attributes to retain classification functionality
  template<typename _Tp>
    inline typename __gnu_cxx::__enable_if<__is_arithmetic<_Tp>::__value,
					   int>::__type
    isnan(_Tp __f) __attribute__ ((optimize ("no-finite-math-only"),noinline));
#endif

  template<typename _Tp>
    inline typename __gnu_cxx::__enable_if<__is_arithmetic<_Tp>::__value,
					   int>::__type
    isnan(_Tp __f)
    {
        return std::__is_integer<_Tp>::__value ? false : __builtin_isnan(__f);
    }

(and obviously similar for fpclassify, isfinite, isinf, and isnormal)

I tweaked isnan to short circuit on __is_integer instead of a roundabout promotion to double.  If you don't like that it can certainly go back to the promotion style.

This works (tested with 4.6.1) because __builtin_isnan is expanded in the isnan context, where the optimize attribute is in effect.  If you think that expansion behavior is subject to change (see also bug 50782), we could bump this up to apply to the builtin itself instead of the user function...?

As written, this relies on the noinline attribute trumping the inline keyword.  We rewrite this to avoid that conflict if needed.  (Is the inline keyword superfluous here anyway? Testing it appears so.)
Comment 36 Eric Gallager 2021-09-21 01:12:36 UTC
This bug is part of a large discussion on the llvm mailing lists, about whether they should do similarly to GCC: https://lists.llvm.org/pipermail/llvm-dev/2021-September/152530.html