Page MenuHomePhabricator

Simplify isfinite/isnan/isinf in finite-math-only mode
AbandonedPublic

Authored by hfinkel on Mar 28 2016, 2:53 AM.

Details

Summary

This patch adds simplification for isfinite, isnan and isinf when we know that we don't have NaNs or Infs (based on the corresponding function attributes). Doing this requires a small infrastructure improvement to TargetLibraryInfo, as I'll explain below.

C/POSIX specify that math.h provides a set of macros, of which these are a subset:

int isfinite(x);
int isnan(x);
int isinf(x);

where x is some floating-point type (float, double, long double). When we're compiling with -ffast-math (or specifically with -ffinite-math-only), it is profitable to statically simplify calls to these functions. For a motivating use case, consider compiling this code using libc++:

#include <complex>
using namespace std;

complex<float> bar(complex<float> C);
complex<float> foo(complex<float> C) {
  return bar(C)*C;
}

and you'll quickly see that, even at -O3 -ffast-math, we produce a mess of code including calls to isnanf and isinff. Why those functions? This comes down to how glibc implements these macros:

  1. define isnan(x) \ (sizeof (x) == sizeof (float) \ ? isnanf (x) \ : sizeof (x) == sizeof (double) \ ? isnan (x) : __isnanl (x))

Other systems use similar macro expansions, with some variation in the names of the underlying functions. OSX here has a split system (or at least used to). When using finite-math-only mode, you get function calls:

#define isfinite(x)                                               \
    ( sizeof(x) == sizeof(float)  ? __isfinitef((float)(x))       \
    : sizeof(x) == sizeof(double) ? __isfinited((double)(x))      \
                                  : __isfinitel((long double)(x)))

but when in IEEE-conforming mode, you get faster inline implementations:

#define isfinite(x)                                                      \
    ( sizeof(x) == sizeof(float)  ? __inline_isfinitef((float)(x))       \
    : sizeof(x) == sizeof(double) ? __inline_isfinited((double)(x))      \
                                  : __inline_isfinitel((long double)(x)))

where the headers define things like:

__header_always_inline int __inline_isfinitef(float __x) {
    return __x == __x && __builtin_fabsf(__x) != __builtin_inff();
}

__header_always_inline int __inline_isinff(float __x) {
    return __builtin_fabsf(__x) == __builtin_inff();
}

__header_always_inline int __inline_isnanf(float __x) {
    return __x != __x;
}

so some effort has been made to preserve the full functioning of these calls even when otherwise compiling in finite-math-only mode. This optimization would purposely break that feature (in favor of lowering abstraction penalties). If we do this and a user wishes to check his or her inputs for NaNs, Infs, etc. the user must do so in a translation unit where such values are permitted to exist. To be fair, gcc's manual does not define finite-math-only mode in this way, but rather:

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

perhaps implying that it is fine to check numbers for NaN/Inf that you did not compute via some arithmetic operation. We could certainly do it this way (i.e. based on an operand's fast-math flags -- although the implementation is not completely trivial because we need to look through PHIs, not just at direct function arguments), although that has the obvious problems with users being surprised by the effects of function inlining. Also, we have -fno-builtin-foo, although it would need some enhancements to work easily in this case because of the macros.

As another data point, FreeBSD seems to always use an inline version of isnan, but has function calls for isinf/isfinite.

Some alternatives (not all mutually exclusive with this one):

  1. As mentioned above, base the folding decision on the fast-math flags of the inputs (looking through phis) instead of the caller's function attributes
  2. In non-finite-math-only mode, replace the calls with a direct implementation (i.e. have the compiler do on all platforms what the OSX math.h header does)
  3. Always replace the calls with inline versions, but mark the instructions somehow so that they don't be removed by later optimizatons
  4. Enhance libc++ to contain some FINITE_MATH_ONLY ifdefs

Regarding the infrastructure enhancement, we currently check for known library calls by name like this in SimplifyLibCalls:

if (TLI->getLibFunc(FuncName, Func) && TLI->has(Func)) {

but currently this does not work if a library function's name is not the default name, but rather one substituted with TLI.setAvailableWithName. This is because we simply never search these custom names when looking for known functions by name (we know only to generate the custom name if we already have its LibFunc identifier). To make this work (necessary for this case because systems disagree on finite vs. isfinite vs. __isfinited, etc.) I've added an additional StringMap to TargetLibraryInfo used to lookup LibFunc identifiers based on custom names. As it turns out, this also requires adding a copy constructor to StringMap (D18506).

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 51760.Mar 28 2016, 2:53 AM
hfinkel retitled this revision from to Simplify isfinite/isnan/isinf in finite-math-only mode.
hfinkel updated this object.
hfinkel added a subscriber: llvm-commits.
hfinkel updated this revision to Diff 51774.Mar 28 2016, 3:23 AM

Updated library function names for Darwin (and NetBSD).

chandlerc edited edge metadata.Mar 30 2016, 1:50 PM
chandlerc added a subscriber: chandlerc.

So, here are my thoughts on this. I'm curious what you, Steven, and others
think though....

My initial feeling is that -ffinite-math-only should apply to *math* and
not to *tests*. That is, we should be free to transform and optimize math
assuming finite operands, but we can't make any assumptions about the
results of testing for finite values. This means that the implementaiton of
complex gets to leverage finite-math-only, but we can't nuke tests for
infinities. I also think that we should aggressively optimize how the tests
are done while preserving their functionality. So I guess I'm suggesting
#2, #3, and #4 from your email as the path forward.

However, I can see an argument that forcing users to leverage
FINITE_MATH_ONLY in their library code is really annoying. So I would
also be happy having two different mechanisms for testing for infinities
(and NaNs, I'm just using the inf case an my example) -- one which is
folded under finite-math-only, and one which survives. I'm not sure what to
call the two interfaces though. Ideas?

In either case, I think we should definitely replace calls to functions
with fast, inline, and ensured correct (according to whatever rules are
appropriate in the particular case) implementations of these tests.

Also in either case, I think we should change the frontend and/or headers
to add *call* attributes (on the actual call instruction) marking these
operations as finite-math-only rather than relying on *caller* attributes
or having to chase operands through phi nodes. This should essentially
follow the same model we use for tagging floating point operations that can
be optimized.

-Chandler

chandlerc requested changes to this revision.Apr 7 2016, 12:19 AM
chandlerc edited edge metadata.

Marking this code itself as awaiting update based on direction comments.

This revision now requires changes to proceed.Apr 7 2016, 12:19 AM
spatel edited edge metadata.Apr 7 2016, 8:07 AM

I haven't thought about the behavioral question raised by -ffinite-math-only, but I wanted to know what the code to perform these ops might look like with default settings.

For reference, these bugs are byproducts of that investigation (although not all will be directly applicable to isfinite/isnan/isinf codegen):
https://llvm.org/bugs/show_bug.cgi?id=27105
https://llvm.org/bugs/show_bug.cgi?id=27145
https://llvm.org/bugs/show_bug.cgi?id=27164
https://llvm.org/bugs/show_bug.cgi?id=27202
https://llvm.org/bugs/show_bug.cgi?id=27203

scanon edited edge metadata.Apr 7 2016, 10:58 AM

Breaking isnan, isinf, etc is a non-starter. I know it's appealing from a consistent formal model viewpoint, but in practice it breaks a lot of code (this is why we have the call fallbacks for iOS/OSX).

I would be delighted to have a finite-math-safe builtin to avoid the calls, but #2 and #4 both seems reasonable to me. #4 should probably be done regardless of the resolution of this conversation.

I agree with Steve here. Even code that is built with -ffast-math has to live in a reality where NaNs, Infs, etc. are sometimes generated by external function calls, provided as user input, etc. It's critical that this code be able to filter out these invalid values, precisely because the body of the code will be optimized on the assumption that those values did not need to be considered.

hfinkel abandoned this revision.Apr 9 2016, 12:07 PM

Breaking isnan, isinf, etc is a non-starter. I know it's appealing from a consistent formal model viewpoint, but in practice it breaks a lot of code (this is why we have the call fallbacks for iOS/OSX).

I would be delighted to have a finite-math-safe builtin to avoid the calls, but #2 and #4 both seems reasonable to me. #4 should probably be done regardless of the resolution of this conversation.

I agree with Steve here. Even code that is built with -ffast-math has to live in a reality where NaNs, Infs, etc. are sometimes generated by external function calls, provided as user input, etc. It's critical that this code be able to filter out these invalid values, precisely because the body of the code will be optimized on the assumption that those values did not need to be considered.

I don't really disagree with any of this. For now, I'll abandon this patch (although I might revive the TLI infrastructure bits, as they're generally useful). I submitted D18639 to fixup libc++ <complex> in this regard.