This is an archive of the discontinued LLVM Phabricator instance.

Use __builtin_isnan/isinf/isfinite in complex
ClosedPublic

Authored by hfinkel on Mar 31 2016, 12:16 AM.

Details

Summary

The libc-provided isnan/isinf/isfinite macro implementations are specifically designed to function correctly, even in the presence of -ffast-math (or, more specifically, -ffinite-math-only). As such, on most implementation, these either always turn into external function calls (e.g. glibc) or are specifically function calls when FINITE_MATH_ONLY is defined (e.g. Darwin).

Our implementation of complex arithmetic make heavy use of isnan/isinf/isfinite to deal with corner cases involving non-finite quantities. This is problematic in two respects:

  1. On systems where these are always function calls (e.g. Linux/glibc), there is a performance penalty
  2. When compiling with -ffast-math, there is a significant performance penalty (in fact, on Darwin and systems with similar implementations, the code may in fact be slower than not using -ffast-math, because the inline definitions provided by libc become unavailable to prevent the checks from being optimized out).

Eliding these inf/nan checks in -ffast-math mode is consistent with what happens with libstdc++, and in my experience, what users expect. This is critical to getting high-performance code when using complex<T>. This patch replaces uses of those functions on basic floating-point types with calls to __builtin_isnan/isinf/isfinite, which Clang will always expand inline. When using -ffast-math (or -ffinite-math-only), the optimizer will remove the checks as expected.

Diff Detail

Repository
rL LLVM

Event Timeline

hfinkel updated this revision to Diff 52178.Mar 31 2016, 12:16 AM
hfinkel retitled this revision from to Use __builtin_isnan/isinf/isfinite in complex.
hfinkel updated this object.
hfinkel added reviewers: mclow.lists, EricWF, chandlerc.
hfinkel added a subscriber: cfe-commits.
spatel added a subscriber: spatel.Apr 10 2016, 2:14 PM

I'm fine with this change, but we should also get Steve to comment on it, and make sure we have a good way of explaining this to users.

In particular, we probably need some documentation around these fast routines that clearly indicates they should only be used when optimizations such as -ffast-math and -ffinite-math-only should strip the checks. We want to be careful to only use the optimizable forms when that behavior is appropriate. And auditing the ones you've switched for that is something Steve might be better suited to do...

I'm fine with this change, but we should also get Steve to comment on it, and make sure we have a good way of explaining this to users.

In particular, we probably need some documentation around these fast routines that clearly indicates they should only be used when optimizations such as -ffast-math and -ffinite-math-only should strip the checks. We want to be careful to only use the optimizable forms when that behavior is appropriate. And auditing the ones you've switched for that is something Steve might be better suited to do...

Also, I spoke to Marshall offline about this last week, and I expect he'll also comment.

mclow.lists edited edge metadata.Jun 30 2016, 8:35 PM

Hal and I talked about this in Oulu. In general, I'm good with this approach.

However, I think that the code could be made much clearer. (some naming changes, some code rearrangement)
First off, I think the name __fast_isinf is a poor name. I think something like __isinf would be better, but Hal informs me that *everyone* uses that name for something, so I think that __libcpp_isinf would be better.

Secondly, instead of duplicating all of the boilerplate code around the different versions of whatever we call it, I think we can bury the ifdefs inside the inline functions. I'll try to work up an example, and post it here.

scanon edited edge metadata.Jul 1 2016, 8:49 AM

I agree with Marshall. Aside from the points he raises, this approach looks fine.

hfinkel updated this revision to Diff 63029.Jul 6 2016, 9:36 PM
hfinkel edited edge metadata.

Thanks everyone. I've rebased this and changed the name to __libcpp_*. Marshall, how do you recommend rewriting the functions to reduce the boilerplate?

I am not the right person to review the C++ template details, but everything else seems OK to me.

Maybe something like this? (untested)

template <class _A1>
_LIBCPP_ALWAYS_INLINE
typename std::enable_if<!std::is_floating_point<_A1>::value, bool>::type
__libcpp_isfinite(_A1 __lcpp_x) _NOEXCEPT
{
    return isfinite(__lcpp_x);
}

template <class _A1>
_LIBCPP_ALWAYS_INLINE
typename std::enable_if<std::is_floating_point<_A1>::value, bool>::type
__libcpp_isfinite(_A1 __lcpp_x) _NOEXCEPT
{
#if __has_builtin(__builtin_isfinite)
    return __builtin_isfinite(__lcpp_x);
#else
    return isfinite(__lcpp_x);
#endif
}

And is there any reason why __libcpp_isinf can't just return false for non-fp types?

And is there any reason why __libcpp_isinf can't just return false for non-fp types?

For custom numeric types that have an isinf, etc. found by ADL, they should continue to work.

hfinkel updated this revision to Diff 67992.Aug 14 2016, 5:35 PM

Updated to use scheme suggested by Marshall.

Updated to use scheme suggested by Marshall.

Ping.

Updated to use scheme suggested by Marshall.

Ping.

Ping.

Updated to use scheme suggested by Marshall.

Ping.

Ping.

Ping.

EricWF accepted this revision.Sep 17 2016, 9:21 PM
EricWF edited edge metadata.

LGTM.

And is there any reason why __libcpp_isinf can't just return false for non-fp types?

For custom numeric types that have an isinf, etc. found by ADL, they should continue to work.

Do we already support custom numeric types? If so could you add a test for this under test/libcxx? Just a simple test case that instantiates the functions and shows it compiles.

include/cmath
593 ↗(On Diff #67992)

nit: the std:: qualifier on types is unnecessary.

This revision is now accepted and ready to land.Sep 17 2016, 9:21 PM

Looks like patch was not committed.

emaste added a subscriber: emaste.Sep 28 2016, 6:47 AM

LGTM.

And is there any reason why __libcpp_isinf can't just return false for non-fp types?

For custom numeric types that have an isinf, etc. found by ADL, they should continue to work.

Do we already support custom numeric types? If so could you add a test for this under test/libcxx? Just a simple test case that instantiates the functions and shows it compiles.

As it turns out, the answer is: mostly. We should discuss this more: PR30589.

This revision was automatically updated to reflect the committed changes.