This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add test for checking progress on P0533R9
ClosedPublic

Authored by Izaron on Oct 22 2022, 1:24 PM.

Details

Summary

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.

Diff Detail

Event Timeline

Izaron created this revision.Oct 22 2022, 1:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2022, 1:24 PM
Izaron requested review of this revision.Oct 22 2022, 1:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2022, 1:24 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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:

  1. A reference list of functions that need to be declared constexpr to make p0533r9 implemented.
  2. 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:

  1. The programmer makes a new constexpr builtin, for example patch for __builtin_fmax.
  2. The programmer has to mark the builtin with the E attribute in Builtins.def, for example __builtins_def (otherwise the evaluator returns early).
  3. Since the builtin __builtin_fmax is marked as constant evaluated (in Builtins.def), the __has_constexpr_builtin(__builtin_fmax) starts returning true.
  4. 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); }
  1. 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
  2. GoTo 1, if there are any other non-constexpr function.
  3. 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.

Izaron updated this revision to Diff 469935.Oct 22 2022, 2:24 PM

Get rid of C++ earlier that C++2b.
Get rid of GNU compiler.

Izaron retitled this revision from [libcxx] Add test for checking progress on P0533R9 to [libc++] Add test for checking progress on P0533R9.Oct 22 2022, 3:21 PM
Izaron edited the summary of this revision. (Show Details)
Izaron updated this revision to Diff 469942.Oct 22 2022, 3:49 PM

Get rid of hexfloat, because hexfloat<long double> not working on test machines.

philnik requested changes to this revision.Oct 23 2022, 4:21 PM

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.

This revision now requires changes to proceed.Oct 23 2022, 4:21 PM
Izaron marked 5 inline comments as done.Oct 24 2022, 10:30 AM
Izaron added inline comments.
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.

Izaron updated this revision to Diff 470209.Oct 24 2022, 10:30 AM
Izaron marked 2 inline comments as done.

Move to constexpr-cxx2b.pass.cpp, fixed issues. Thanks to @philnik!

Izaron added a comment.EditedOct 24 2022, 10:42 AM

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); }

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.

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

__config
#ifndef __has_constexpr_builtin
#  define __has_constexpr_builtin(...) 0
#endif
math.h
#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.

Izaron updated this revision to Diff 471880.Oct 30 2022, 3:00 PM

Add test for GCC. Thanks to @philnik!

philnik accepted this revision.Oct 30 2022, 3:04 PM

LGTM with CI passing.

This revision is now accepted and ready to land.Oct 30 2022, 3:04 PM
Izaron updated this revision to Diff 472062.Oct 31 2022, 10:41 AM

Fix GCC tests - some of its functions are already constexpr

This revision was automatically updated to reflect the committed changes.

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.

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.