This test checks progress on P0533R9 (https://wg21.link/p0533r9).
Whenever a desired function becomes constexpr, the programmer won't forget
to switch ASSERT_NOT_CONSTEXPR_CXX23 to ASSERT_CONSTEXPR_CXX23 and
eventually to change the paper's implementation status. The test also works
as a reference list of unimplemented functions.
Details
- Reviewers
aaron.ballman philnik cor3ntin ldionne - Group Reviewers
Restricted Project - Commits
- rGcc2cf8b22b35: [libc++] Add test for checking progress on P0533R9
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This is a part of our work on implementing p0533r9:
https://discourse.llvm.org/t/how-do-we-plan-to-make-constexpr-cmath-and-cstdlib/65930
This patch introduces a test that serve as:
- A reference list of functions that need to be declared constexpr to make p0533r9 implemented.
- A progress checker, so that whenever a new function is declared constexpr, the programmer won't forget to switch the corresponding ASSERT_NOT_CONSTEXPR_CXX23s to ASSERT_CONSTEXPR_CXX23s and eventually confidently mark the paper as implemented.
With this test (and previous commits) we will have a nice automated workflow for working on this paper:
- The programmer makes a new constexpr builtin, for example patch for __builtin_fmax.
- The programmer has to mark the builtin with the E attribute in Builtins.def, for example __builtins_def (otherwise the evaluator returns early).
- Since the builtin __builtin_fmax is marked as constant evaluated (in Builtins.def), the __has_constexpr_builtin(__builtin_fmax) starts returning true.
- Since __has_constexpr_builtin(__builtin_fmax) == true, the corresponding libcxx function std::fmax becomes constexpr, because our fellow @philnik is now refactoring cmath to using builtins unconditionally, and the functions will be conditionally constexpr, so it will soon look like this:
inline _LIBCPP_CONSTEXPR_CXX23_IF_CONSTEXPR_BUILTIN(__builtin_fmax) _LIBCPP_HIDE_FROM_ABI float fmax(float __x, float __y) _NOEXCEPT { return __builtin_fmax(__x, __y); }
- The new test (that I added in this patch request) starts to fails, so the programmer has to switch ASSERT_NOT_CONSTEXPR_CXX23 to ASSERT_CONSTEXPR_CXX23
- GoTo 1, if there are any other non-constexpr function.
- There are no non-constexpr functions anymore, so the new test (that I added in this patch request) congratulates you! The paper is implemented.
I hope this workflow is clear 😅 This is not an overengineering - only a clear adherence to the paper so that we clearly understand the current status.
Thanks for working on this! Looks good in general, but I've got a few suggestions.
libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp | ||
---|---|---|
1 ↗ | (On Diff #469942) | I think this test should be in libcxx/test/libcxx, since we don't actually test the standard, but our current implementation status. |
2 ↗ | (On Diff #469942) | Could you also add a test for GCC and the global namespace versions of the functions? |
13 ↗ | (On Diff #469942) | What do you need this include for? |
16 ↗ | (On Diff #469942) | You can mark the test as // REQUIRES: clang and // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20 instead. |
19 ↗ | (On Diff #469942) | What does _builtin_constant_p do? |
30 ↗ | (On Diff #469942) | I'm not sure these comments add a lot of value, since it's pretty clear what is tested by looking at the call. |
libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp | ||
---|---|---|
2 ↗ | (On Diff #469942) | What are "global namespace versions of the functions"? Functions outside the std:: namespace? |
13 ↗ | (On Diff #469942) | I couldn't use TEST_STD_VER without this include. But now this is irrelevant since I don't need this macro with // REQUIRES and // UNSUPPORTED. I don't include this anymore. |
19 ↗ | (On Diff #469942) | __builtin_constant_p(Expr) is a GNU builtin (has been ported to Clang) that determines whether the Expr expression is known to be constant in compile time - https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html This is somewhat similar with __has_constexpr_builtin: static_assert(__builtin_constant_p(__builtin_fmax(12.34, 45.67) == 45.67)); static_assert(__has_constexpr_builtin(__builtin_fmax)); I think __has_constexpr_builtin is better for "conditionally constexpr" math functions in the headers, and __builtin_constant_p is better for tests checking that expression is constant evaluable. ASSERT_CONSTEXPR_CXX23 ensures that the Expr is constant evaluable, ASSERT_NOT_CONSTEXPR_CXX23 ensures that isn't. |
Could you also add a test for GCC and the global namespace versions of the functions?
Does "test for GCC" mean testing how GCC would compile libc++?
I think we could make it by copy-pasting the file with changing // REQUIRES: clang to // REQUIRES: gcc.
But we need to think how we could declare functions constexpr giving that GCC doesn't support __has_constexpr_builtin? Since GCC supposedly supports a ton of constexpr builtins now, we could check __has_constexpr_builtin for Clang and the compiler's version for GCC. Would that work?
#ifdef __clang__ # if __has_constexpr_builtin(__builtin_fmax) constexpr # endif #elifdef __GNU__ # if __GNU_VERSION__ >= 1234 constexpr # endif #endif double fmax(double a, double b) { return __builtin_fmax(a, b); }
Yes, that's what I had in mind.
But we need to think how we could declare functions constexpr giving that GCC doesn't support __has_constexpr_builtin? Since GCC supposedly supports a ton of constexpr builtins now, we could check __has_constexpr_builtin for Clang and the compiler's version for GCC. Would that work?
#ifdef __clang__ # if __has_constexpr_builtin(__builtin_fmax) constexpr # endif #elifdef __GNU__ # if __GNU_VERSION__ >= 1234 constexpr # endif #endif double fmax(double a, double b) { return __builtin_fmax(a, b); }
We only support GCC 12, so we could simplify this to
#ifndef __has_constexpr_builtin # define __has_constexpr_builtin(...) 0 #endif
#if __has_constexpr_builtin(__builtin_fmax) || defined(_LIBCPP_COMPILER_GCC) constexpr #endif double fmax(double a, double b) { return __builtin_fmax(a, b); }
but yes, that's the general idea.
libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp | ||
---|---|---|
2 ↗ | (On Diff #469942) | yes |
13 ↗ | (On Diff #469942) | I missed that, thanks. |
Thanks for the contribution. However, this looks a bit unusual to me. @philnik I think this should really be under libcxx/test/std since we are testing standard conformance. We would comment out tests for functions that we don't implement constexpr for yet, and uncomment them as we go. This is what we've done a bunch in e.g. ranges.
I'd consider this test to be a bit different. Here we essentially check what the compiler has implemented. We don't have to do anything special in the implementation, we just have to add constexpr to the functions. My plan was to add tests as we mark the functions constexpr, since our current cmath tests are really lacking. It currently also differs massively between the compilers what we can actually make constexpr, which isn't normally the case AFAIK.