Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
Eugene.Zelenko added a subscriber: Eugene.Zelenko. Eugene.Zelenko added a comment. Looks like patch was not committed. https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
EricWF accepted this revision. EricWF added a comment. This revision is now accepted and ready to land. LGTM. In https://reviews.llvm.org/D18639#514991, @hfinkel wrote: > In https://reviews.llvm.org/D18639#491232, @mclow.lists wrote: > > > 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. Comment at: include/cmath:593 @@ +592,3 @@ +_LIBCPP_ALWAYS_INLINE +typename std::enable_if::value, bool>::type +__libcpp_isnan(_A1 __lcpp_x) _NOEXCEPT nit: the `std::` qualifier on types is unnecessary. https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel added a comment. In https://reviews.llvm.org/D18639#533807, @hfinkel wrote: > In https://reviews.llvm.org/D18639#527285, @hfinkel wrote: > > > In https://reviews.llvm.org/D18639#515000, @hfinkel wrote: > > > > > Updated to use scheme suggested by Marshall. > > > > > > Ping. > > > Ping. Ping. https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel added a comment. In https://reviews.llvm.org/D18639#527285, @hfinkel wrote: > In https://reviews.llvm.org/D18639#515000, @hfinkel wrote: > > > Updated to use scheme suggested by Marshall. > > > Ping. Ping. https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel added a comment. In https://reviews.llvm.org/D18639#515000, @hfinkel wrote: > Updated to use scheme suggested by Marshall. Ping. https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel updated this revision to Diff 67992.
hfinkel added a comment.
Updated to use scheme suggested by Marshall.
https://reviews.llvm.org/D18639
Files:
include/cmath
include/complex
Index: include/complex
===
--- include/complex
+++ include/complex
@@ -602,39 +602,39 @@
_Tp __bc = __b * __c;
_Tp __x = __ac - __bd;
_Tp __y = __ad + __bc;
-if (isnan(__x) && isnan(__y))
+if (__libcpp_isnan(__x) && __libcpp_isnan(__y))
{
bool __recalc = false;
-if (isinf(__a) || isinf(__b))
+if (__libcpp_isinf(__a) || __libcpp_isinf(__b))
{
-__a = copysign(isinf(__a) ? _Tp(1) : _Tp(0), __a);
-__b = copysign(isinf(__b) ? _Tp(1) : _Tp(0), __b);
-if (isnan(__c))
+__a = copysign(__libcpp_isinf(__a) ? _Tp(1) : _Tp(0), __a);
+__b = copysign(__libcpp_isinf(__b) ? _Tp(1) : _Tp(0), __b);
+if (__libcpp_isnan(__c))
__c = copysign(_Tp(0), __c);
-if (isnan(__d))
+if (__libcpp_isnan(__d))
__d = copysign(_Tp(0), __d);
__recalc = true;
}
-if (isinf(__c) || isinf(__d))
+if (__libcpp_isinf(__c) || __libcpp_isinf(__d))
{
-__c = copysign(isinf(__c) ? _Tp(1) : _Tp(0), __c);
-__d = copysign(isinf(__d) ? _Tp(1) : _Tp(0), __d);
-if (isnan(__a))
+__c = copysign(__libcpp_isinf(__c) ? _Tp(1) : _Tp(0), __c);
+__d = copysign(__libcpp_isinf(__d) ? _Tp(1) : _Tp(0), __d);
+if (__libcpp_isnan(__a))
__a = copysign(_Tp(0), __a);
-if (isnan(__b))
+if (__libcpp_isnan(__b))
__b = copysign(_Tp(0), __b);
__recalc = true;
}
-if (!__recalc && (isinf(__ac) || isinf(__bd) ||
- isinf(__ad) || isinf(__bc)))
+if (!__recalc && (__libcpp_isinf(__ac) || __libcpp_isinf(__bd) ||
+ __libcpp_isinf(__ad) || __libcpp_isinf(__bc)))
{
-if (isnan(__a))
+if (__libcpp_isnan(__a))
__a = copysign(_Tp(0), __a);
-if (isnan(__b))
+if (__libcpp_isnan(__b))
__b = copysign(_Tp(0), __b);
-if (isnan(__c))
+if (__libcpp_isnan(__c))
__c = copysign(_Tp(0), __c);
-if (isnan(__d))
+if (__libcpp_isnan(__d))
__d = copysign(_Tp(0), __d);
__recalc = true;
}
@@ -677,33 +677,33 @@
_Tp __c = __w.real();
_Tp __d = __w.imag();
_Tp __logbw = logb(fmax(fabs(__c), fabs(__d)));
-if (isfinite(__logbw))
+if (__libcpp_isfinite(__logbw))
{
__ilogbw = static_cast(__logbw);
__c = scalbn(__c, -__ilogbw);
__d = scalbn(__d, -__ilogbw);
}
_Tp __denom = __c * __c + __d * __d;
_Tp __x = scalbn((__a * __c + __b * __d) / __denom, -__ilogbw);
_Tp __y = scalbn((__b * __c - __a * __d) / __denom, -__ilogbw);
-if (isnan(__x) && isnan(__y))
+if (__libcpp_isnan(__x) && __libcpp_isnan(__y))
{
-if ((__denom == _Tp(0)) && (!isnan(__a) || !isnan(__b)))
+if ((__denom == _Tp(0)) && (!__libcpp_isnan(__a) || !__libcpp_isnan(__b)))
{
__x = copysign(_Tp(INFINITY), __c) * __a;
__y = copysign(_Tp(INFINITY), __c) * __b;
}
-else if ((isinf(__a) || isinf(__b)) && isfinite(__c) && isfinite(__d))
+else if ((__libcpp_isinf(__a) || __libcpp_isinf(__b)) && __libcpp_isfinite(__c) && __libcpp_isfinite(__d))
{
-__a = copysign(isinf(__a) ? _Tp(1) : _Tp(0), __a);
-__b = copysign(isinf(__b) ? _Tp(1) : _Tp(0), __b);
+__a = copysign(__libcpp_isinf(__a) ? _Tp(1) : _Tp(0), __a);
+__b = copysign(__libcpp_isinf(__b) ? _Tp(1) : _Tp(0), __b);
__x = _Tp(INFINITY) * (__a * __c + __b * __d);
__y = _Tp(INFINITY) * (__b * __c - __a * __d);
}
-else if (isinf(__logbw) && __logbw > _Tp(0) && isfinite(__a) && isfinite(__b))
+else if (__libcpp_isinf(__logbw) && __logbw > _Tp(0) && __libcpp_isfinite(__a) && __libcpp_isfinite(__b))
{
-__c = copysign(isinf(__c) ? _Tp(1) : _Tp(0), __c);
-__d = copysign(isinf(__d) ? _Tp(1) : _Tp(0), __d);
+__c = copysign(__libcpp_isinf(__c) ? _Tp(1) : _Tp(0), __c);
+__d = copysign(__libcpp_isinf(__d) ? _Tp(1) : _Tp(0), __d);
__x = _Tp(0) * (__a * __c + __b * __d);
__y = _Tp(0) * (__b * __c - __a * __d);
}
@@ -913,9 +913,9 @@
_Tp
norm(const complex<_Tp>& __c)
{
-if (isinf(__c.real()))
+if (__libcpp_isinf(__c.real()))
return abs(__c.real());
-if (isinf(__c.imag()))
+if (__libcpp_isinf(__c.imag()))
return abs(__c.imag());
return __c.real() *
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel added a comment. In https://reviews.llvm.org/D18639#491232, @mclow.lists wrote: > 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. https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
mclow.lists added a comment. And is there any reason why `__libcpp_isinf` can't just return `false` for non-fp types? https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
mclow.lists added a comment.
Maybe something like this? (untested)
template
_LIBCPP_ALWAYS_INLINE
typename std::enable_if::value, bool>::type
__libcpp_isfinite(_A1 __lcpp_x) _NOEXCEPT
{
return isfinite(__lcpp_x);
}
template
_LIBCPP_ALWAYS_INLINE
typename std::enable_if::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
}
https://reviews.llvm.org/D18639
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
scanon added a comment. I am not the right person to review the C++ template details, but everything else seems OK to me. https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel added a comment. Ping. https://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel updated this revision to Diff 63029.
hfinkel added a comment.
Thanks everyone. I've rebased this and changed the name to __libcpp_*.
Marshall, how do you recommend rewriting the functions to reduce the
boilerplate?
http://reviews.llvm.org/D18639
Files:
include/cmath
include/complex
Index: include/complex
===
--- include/complex
+++ include/complex
@@ -602,39 +602,39 @@
_Tp __bc = __b * __c;
_Tp __x = __ac - __bd;
_Tp __y = __ad + __bc;
-if (isnan(__x) && isnan(__y))
+if (__libcpp_isnan(__x) && __libcpp_isnan(__y))
{
bool __recalc = false;
-if (isinf(__a) || isinf(__b))
+if (__libcpp_isinf(__a) || __libcpp_isinf(__b))
{
-__a = copysign(isinf(__a) ? _Tp(1) : _Tp(0), __a);
-__b = copysign(isinf(__b) ? _Tp(1) : _Tp(0), __b);
-if (isnan(__c))
+__a = copysign(__libcpp_isinf(__a) ? _Tp(1) : _Tp(0), __a);
+__b = copysign(__libcpp_isinf(__b) ? _Tp(1) : _Tp(0), __b);
+if (__libcpp_isnan(__c))
__c = copysign(_Tp(0), __c);
-if (isnan(__d))
+if (__libcpp_isnan(__d))
__d = copysign(_Tp(0), __d);
__recalc = true;
}
-if (isinf(__c) || isinf(__d))
+if (__libcpp_isinf(__c) || __libcpp_isinf(__d))
{
-__c = copysign(isinf(__c) ? _Tp(1) : _Tp(0), __c);
-__d = copysign(isinf(__d) ? _Tp(1) : _Tp(0), __d);
-if (isnan(__a))
+__c = copysign(__libcpp_isinf(__c) ? _Tp(1) : _Tp(0), __c);
+__d = copysign(__libcpp_isinf(__d) ? _Tp(1) : _Tp(0), __d);
+if (__libcpp_isnan(__a))
__a = copysign(_Tp(0), __a);
-if (isnan(__b))
+if (__libcpp_isnan(__b))
__b = copysign(_Tp(0), __b);
__recalc = true;
}
-if (!__recalc && (isinf(__ac) || isinf(__bd) ||
- isinf(__ad) || isinf(__bc)))
+if (!__recalc && (__libcpp_isinf(__ac) || __libcpp_isinf(__bd) ||
+ __libcpp_isinf(__ad) || __libcpp_isinf(__bc)))
{
-if (isnan(__a))
+if (__libcpp_isnan(__a))
__a = copysign(_Tp(0), __a);
-if (isnan(__b))
+if (__libcpp_isnan(__b))
__b = copysign(_Tp(0), __b);
-if (isnan(__c))
+if (__libcpp_isnan(__c))
__c = copysign(_Tp(0), __c);
-if (isnan(__d))
+if (__libcpp_isnan(__d))
__d = copysign(_Tp(0), __d);
__recalc = true;
}
@@ -677,33 +677,33 @@
_Tp __c = __w.real();
_Tp __d = __w.imag();
_Tp __logbw = logb(fmax(fabs(__c), fabs(__d)));
-if (isfinite(__logbw))
+if (__libcpp_isfinite(__logbw))
{
__ilogbw = static_cast(__logbw);
__c = scalbn(__c, -__ilogbw);
__d = scalbn(__d, -__ilogbw);
}
_Tp __denom = __c * __c + __d * __d;
_Tp __x = scalbn((__a * __c + __b * __d) / __denom, -__ilogbw);
_Tp __y = scalbn((__b * __c - __a * __d) / __denom, -__ilogbw);
-if (isnan(__x) && isnan(__y))
+if (__libcpp_isnan(__x) && __libcpp_isnan(__y))
{
-if ((__denom == _Tp(0)) && (!isnan(__a) || !isnan(__b)))
+if ((__denom == _Tp(0)) && (!__libcpp_isnan(__a) || !__libcpp_isnan(__b)))
{
__x = copysign(_Tp(INFINITY), __c) * __a;
__y = copysign(_Tp(INFINITY), __c) * __b;
}
-else if ((isinf(__a) || isinf(__b)) && isfinite(__c) && isfinite(__d))
+else if ((__libcpp_isinf(__a) || __libcpp_isinf(__b)) && __libcpp_isfinite(__c) && __libcpp_isfinite(__d))
{
-__a = copysign(isinf(__a) ? _Tp(1) : _Tp(0), __a);
-__b = copysign(isinf(__b) ? _Tp(1) : _Tp(0), __b);
+__a = copysign(__libcpp_isinf(__a) ? _Tp(1) : _Tp(0), __a);
+__b = copysign(__libcpp_isinf(__b) ? _Tp(1) : _Tp(0), __b);
__x = _Tp(INFINITY) * (__a * __c + __b * __d);
__y = _Tp(INFINITY) * (__b * __c - __a * __d);
}
-else if (isinf(__logbw) && __logbw > _Tp(0) && isfinite(__a) && isfinite(__b))
+else if (__libcpp_isinf(__logbw) && __logbw > _Tp(0) && __libcpp_isfinite(__a) && __libcpp_isfinite(__b))
{
-__c = copysign(isinf(__c) ? _Tp(1) : _Tp(0), __c);
-__d = copysign(isinf(__d) ? _Tp(1) : _Tp(0), __d);
+__c = copysign(__libcpp_isinf(__c) ? _Tp(1) : _Tp(0), __c);
+__d = copysign(__libcpp_isinf(__d) ? _Tp(1) : _Tp(0), __d);
__x = _Tp(0) * (__a * __c + __b * __d);
__y = _Tp(0) * (__b * __c - __a * __d);
}
@@ -941,9 +941,9 @@
_Tp
norm(const complex<_Tp>& __c)
{
-if (isinf(__c.real()))
+if (__libcpp_isinf(__c.real()))
return abs(__c.real());
-if (isin
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
scanon added a comment. I agree with Marshall. Aside from the points he raises, this approach looks fine. http://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
mclow.lists added a comment. 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. http://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel added a comment. In http://reviews.llvm.org/D18639#472010, @chandlerc wrote: > 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. http://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
chandlerc added a reviewer: scanon. chandlerc added a comment. 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... http://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18639: Use __builtin_isnan/isinf/isfinite in complex
hfinkel added a comment. ping http://reviews.llvm.org/D18639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
