Skip to content

Conversation

@petrhosek
Copy link
Member

lgamma_r is currently only available on GPU targets.

@petrhosek petrhosek added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libc labels Sep 22, 2024
@petrhosek petrhosek requested a review from lntue September 22, 2024 02:12
@petrhosek petrhosek requested a review from a team as a code owner September 22, 2024 02:12
@llvmbot
Copy link
Member

llvmbot commented Sep 22, 2024

@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-libc

Author: Petr Hosek (petrhosek)

Changes

lgamma_r is currently only available on GPU targets.


Full diff: https://github.com/llvm/llvm-project/pull/109556.diff

1 Files Affected:

  • (modified) libcxx/include/__random/binomial_distribution.h (+1-1)
diff --git a/libcxx/include/__random/binomial_distribution.h b/libcxx/include/__random/binomial_distribution.h
index 3f19746bae238c..9538c15e2dc97b 100644
--- a/libcxx/include/__random/binomial_distribution.h
+++ b/libcxx/include/__random/binomial_distribution.h
@@ -103,7 +103,7 @@ extern "C" double lgamma_r(double, int*);
 #endif
 
 inline _LIBCPP_HIDE_FROM_ABI double __libcpp_lgamma(double __d) {
-#if defined(_LIBCPP_MSVCRT_LIKE)
+#if defined(_LIBCPP_MSVCRT_LIKE) || defined(__LLVM_LIBC__)
   return lgamma(__d);
 #else
   int __sign;

@jhuber6
Copy link
Contributor

jhuber6 commented Sep 22, 2024

But we don't have lgamma either for non-GPU targets, so how does this work?

@petrhosek
Copy link
Member Author

But we don't have lgamma either for non-GPU targets, so how does this work?

libc++ provides it's own implementation which uses __builtin_lgamma, see:

inline _LIBCPP_HIDE_FROM_ABI float lgamma(float __x) _NOEXCEPT { return __builtin_lgammaf(__x); }
template <class = int>
_LIBCPP_HIDE_FROM_ABI double lgamma(double __x) _NOEXCEPT {
return __builtin_lgamma(__x);
}
inline _LIBCPP_HIDE_FROM_ABI long double lgamma(long double __x) _NOEXCEPT { return __builtin_lgammal(__x); }
template <class _A1, __enable_if_t<is_integral<_A1>::value, int> = 0>
inline _LIBCPP_HIDE_FROM_ABI double lgamma(_A1 __x) _NOEXCEPT {
return __builtin_lgamma((double)__x);
}

@jhuber6
Copy link
Contributor

jhuber6 commented Sep 22, 2024

But we don't have lgamma either for non-GPU targets, so how does this work?

libc++ provides it's own implementation which uses __builtin_lgamma, see:

inline _LIBCPP_HIDE_FROM_ABI float lgamma(float __x) _NOEXCEPT { return __builtin_lgammaf(__x); }
template <class = int>
_LIBCPP_HIDE_FROM_ABI double lgamma(double __x) _NOEXCEPT {
return __builtin_lgamma(__x);
}
inline _LIBCPP_HIDE_FROM_ABI long double lgamma(long double __x) _NOEXCEPT { return __builtin_lgammal(__x); }
template <class _A1, __enable_if_t<is_integral<_A1>::value, int> = 0>
inline _LIBCPP_HIDE_FROM_ABI double lgamma(_A1 __x) _NOEXCEPT {
return __builtin_lgamma((double)__x);
}

__builtin_lgamma isn't handled anywhere in the backend AFAIK, it will almost certainly be reduced to an unresolved libcall.

@ldionne
Copy link
Member

ldionne commented Sep 25, 2024

Please rebase onto main to re-trigger CI. The CI instability should be resolved now.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ green CI (please rebase).

lgamma_r is currently only available on GPU targets.
@ldionne ldionne force-pushed the libcxx-libc-lgamma branch from a9376b9 to c34a87b Compare October 1, 2024 20:21
@ldionne
Copy link
Member

ldionne commented Oct 1, 2024

I rebased. This can be landed once CI is green.

@jhuber6
Copy link
Contributor

jhuber6 commented Oct 1, 2024

I'm fine with this in general, since I think once the LLVM libc implements lgamma it is probably fine to not provide the signgam external global. This isn't mentioned in the standard, so it's probably an extension GNU / POSIX made than then had to make lgamma_r to escape from once people started realizing global state was bad. I'm just saying that neither lgamma nor lgamma_r are provided right now, the builtin will just make the symbol undefined (but I guess you can compile it?) It's the same as if you just put double lgamma(double) in the file without ever definint it.

@petrhosek petrhosek merged commit 84ce230 into llvm:main Nov 6, 2024
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. libc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants