[CUDA] Mark __libcpp_{isnan,isinf,isfinite} as constexpr.

[jlebar]

Summary

This makes these functions available on host and device, which is
necessary to compile <complex> for the device.

[jlebar]

Although these pass the CUDA test-suite tests (which I haven't yet committed because they're broken without this change), I could use some help running the libcxx tests.

I cannot find any documentation explaining how to run the libcxx tests with just-built clang. Presumably this is supported and I just cannot figure it out. Anyway I figured I'd try to make a new objdir and point it to my just-built clang.

$ mkdir objdir-libcxx
$ cd objdir-libcxx
$ cmake -G "Ninja" -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=$HOME/llvm/release/bin/clang -DCMAKE_CXX_COMPILER=$HOME/llvm/release/bin/clang -DLLVM_ENABLE_ASSERTIONS=On ../llvm

This also doesn't work. Building one of cmake's atomic tests fails with linker errors.

Run Build Command:"/usr/local/google/home/jlebar/bin/ninja" "cmTC_0c40a"
[1/2] Building CXX object CMakeFiles/cmTC_0c40a.dir/src.cxx.o
[2/2] Linking CXX executable cmTC_0c40a
FAILED: cmTC_0c40a
: && /usr/local/google/home/jlebar/llvm/release/bin/clang   -DHAVE_CXX_ATOMICS_WITH_LIB -std=c++11   CMakeFiles/cmTC_0c40a.dir/src.cxx.o  -o cmTC_0c40a  -lm -latomic && :
CMakeFiles/cmTC_0c40a.dir/src.cxx.o:src.cxx:function __clang_call_terminate: error: undefined reference to '__cxa_begin_catch'
CMakeFiles/cmTC_0c40a.dir/src.cxx.o:src.cxx:function __clang_call_terminate: error: undefined reference to 'std::terminate()'
CMakeFiles/cmTC_0c40a.dir/src.cxx.o(.eh_frame+0x143): error: undefined reference to '__gxx_personality_v0'
clang-4.0: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Source file was:

#include <atomic>
std::atomic<int> x;
int main() {
  return x;
}

I presume there is some Right Way to do this, and I'm just not doing it.

[hfinkel]

In D25403#565603, @jlebar wrote:

Although these pass the CUDA test-suite tests (which I haven't yet committed because they're broken without this change), I could use some help running the libcxx tests.

I cannot find any documentation explaining how to run the libcxx tests with just-built clang. Presumably this is supported and I just cannot figure it out. Anyway I figured I'd try to make a new objdir and point it to my just-built clang.

$ mkdir objdir-libcxx
$ cd objdir-libcxx
$ cmake -G "Ninja" -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=$HOME/llvm/release/bin/clang -DCMAKE_CXX_COMPILER=$HOME/llvm/release/bin/clang -DLLVM_ENABLE_ASSERTIONS=On ../llvm

This also doesn't work. Building one of cmake's atomic tests fails with linker errors.

Run Build Command:"/usr/local/google/home/jlebar/bin/ninja" "cmTC_0c40a"
[1/2] Building CXX object CMakeFiles/cmTC_0c40a.dir/src.cxx.o
[2/2] Linking CXX executable cmTC_0c40a
FAILED: cmTC_0c40a
: && /usr/local/google/home/jlebar/llvm/release/bin/clang   -DHAVE_CXX_ATOMICS_WITH_LIB -std=c++11   CMakeFiles/cmTC_0c40a.dir/src.cxx.o  -o cmTC_0c40a  -lm -latomic && :
CMakeFiles/cmTC_0c40a.dir/src.cxx.o:src.cxx:function __clang_call_terminate: error: undefined reference to '__cxa_begin_catch'
CMakeFiles/cmTC_0c40a.dir/src.cxx.o:src.cxx:function __clang_call_terminate: error: undefined reference to 'std::terminate()'
CMakeFiles/cmTC_0c40a.dir/src.cxx.o(.eh_frame+0x143): error: undefined reference to '__gxx_personality_v0'
clang-4.0: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Source file was:

#include <atomic>
std::atomic<int> x;
int main() {
  return x;
}

I presume there is some Right Way to do this, and I'm just not doing it.

The tests should be runnable with lit. I generally just do an in-tree build and run make check-libcxx. @EricWF , what's the recommended way of running the tests from an out-of-tree build?

[jlebar]

The tests should be runnable with lit. I generally just do an in-tree build and run make check-libcxx. @EricWF , what's the recommended way of running the tests from an out-of-tree build?

Things seem broken at the moment with clang from tip of tree.

I did a clean in-tree build and then modified the config.cxx_under_test line in projects/libcxx/test/lit.site.cfg to point to my newly-built clang. Running check-cxx gets tons of failing tests [1]. However, if I change that to point to my clang-3.8 binary, everything works fine, with and without this patch here.

So, issues with libc++ and current clang aside (I'm happy to file a bug somewhere?), this patch seems to pass the tests just fine.

Would be nice to get this in -- it's the only thing blocking me from turning on these tests in the test suite (and announcing full support for std::complex on the device).

[1] One example of a failing test:

---

  FAIL: libc++ :: std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp (732 of 5634)
  ******************** TEST 'libc++ :: std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp' FAILED ********************
  Command: ['/usr/local/google/home/jlebar/llvm/release/bin/clang++', '-o', '/usr/local/google/home/jlebar/code/llvm/release/projects/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/Output/ctor_move.pass.cpp.o', '-x', 'c++', '/usr/local/google/home/jlebar/code/llvm/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp', '-c', '-v', '-Werror=thread-safety', '-std=c++1z', '-include', '/usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/test/support/nasty_macros.hpp', '-nostdinc++', '-I/usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include', '-D__STDC_FORMAT_MACROS', '-D__STDC_LIMIT_MACROS', '-D__STDC_CONSTANT_MACROS', '-I/usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/test/support', '-DLIBCXX_FILESYSTEM_STATIC_TEST_ROOT="/usr/local/google/home/jlebar/code/llvm/libcxx/test/std/experimental/filesystem/Inputs/static_test_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT="/usr/local/google/home/jlebar/code/llvm/release/projects/libcxx/test/filesystem/Output/dynamic_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER="/usr/bin/python2.7 /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/test/support/filesystem_dynamic_test_helper.py"', '-c']
  Exit Code: 1
  Standard Error:
  --
  clang version 4.0.0 
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: /usr/local/google/home/jlebar/llvm/release/bin
  Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.7
  Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.7.3
  Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8
  Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8.4
  Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9
  Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9.3
  Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8
  Candidate multilib: .;@m64
  Candidate multilib: 32;@m32
  Candidate multilib: x32;@mx32
  Selected multilib: .;@m64
  Found CUDA installation: /usr/local/cuda, version 7.0
   "/usr/local/google/home/jlebar/code/llvm/release/bin/clang-4.0" -cc1 -triple x86_64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -main-file-name ctor_move.pass.cpp -mrelocation-model static -mthread-model posix -mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -v -dwarf-column-info -debugger-tuning=gdb -coverage-notes-file /usr/local/google/home/jlebar/code/llvm/release/projects/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/Output/ctor_move.pass.cpp.gcno -nostdinc++ -resource-dir /usr/local/google/home/jlebar/code/llvm/release/bin/../lib/clang/4.0.0 -include /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/test/support/nasty_macros.hpp -I /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include -D __STDC_FORMAT_MACROS -D __STDC_LIMIT_MACROS -D __STDC_CONSTANT_MACROS -I /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/test/support -D "LIBCXX_FILESYSTEM_STATIC_TEST_ROOT=\"/usr/local/google/home/jlebar/code/llvm/libcxx/test/std/experimental/filesystem/Inputs/static_test_env\"" -D "LIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT=\"/usr/local/google/home/jlebar/code/llvm/release/projects/libcxx/test/filesystem/Output/dynamic_env\"" -D "LIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER=\"/usr/bin/python2.7 /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/test/support/filesystem_dynamic_test_helper.py\"" -internal-isystem /usr/local/include -internal-isystem /usr/local/google/home/jlebar/code/llvm/release/bin/../lib/clang/4.0.0/include -internal-externc-isystem /usr/include/x86_64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -Werror=thread-safety -std=c++1z -fdeprecated-macro -fdebug-compilation-dir /usr/local/google/home/jlebar/code/llvm/release/projects/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons -ferror-limit 19 -fmessage-length 0 -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -o /usr/local/google/home/jlebar/code/llvm/release/projects/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/Output/ctor_move.pass.cpp.o -x c++ /usr/local/google/home/jlebar/code/llvm/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp
  clang -cc1 version 4.0.0 based upon LLVM 4.0.0svn default target x86_64-unknown-linux-gnu
  ignoring nonexistent directory "/include"
  #include "..." search starts here:
  #include <...> search starts here:
   /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include
   /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/test/support
   /usr/local/include
   /usr/local/google/home/jlebar/code/llvm/release/bin/../lib/clang/4.0.0/include
   /usr/include/x86_64-linux-gnu
   /usr/include
  End of search list.
  In file included from /usr/local/google/home/jlebar/code/llvm/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp:14:
  In file included from /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/queue:170:
  In file included from /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:158:
  In file included from /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:7:
  In file included from /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/algorithm:639:
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/utility:260:1: error: conflicting types for 'swap'
  swap(_Tp (&__a)[_Np], _Tp (&__b)[_Np]) _NOEXCEPT_(__is_nothrow_swappable<_Tp>::value)
  ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/type_traits:4381:1: note: previous declaration is here
  swap(_Tp (&__a)[_Np], _Tp (&__b)[_Np]) _NOEXCEPT_(__is_nothrow_swappable<_Tp>::value);
  ^
  In file included from /usr/local/google/home/jlebar/code/llvm/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp:14:
  In file included from /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/queue:170:
  In file included from /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:158:
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:318:34: error: conflicting types for '__split_buffer<_Tp, _Allocator>'
  __split_buffer<_Tp, _Allocator>::__split_buffer()
				   ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:60:5: note: previous declaration is here
      __split_buffer()
      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:349:34: error: conflicting types for '__split_buffer<_Tp, _Allocator>'
  __split_buffer<_Tp, _Allocator>::__split_buffer(__split_buffer&& __c)
				   ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:70:5: note: previous declaration is here
      __split_buffer(__split_buffer&& __c)
      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:390:34: error: conflicting types for 'operator='
  __split_buffer<_Tp, _Allocator>::operator=(__split_buffer&& __c)
				   ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:73:21: note: previous declaration is here
      __split_buffer& operator=(__split_buffer&& __c)
		      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:412:34: error: conflicting types for 'swap'
  __split_buffer<_Tp, _Allocator>::swap(__split_buffer& __x)
				   ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/__split_buffer:147:10: note: previous declaration is here
      void swap(__split_buffer& __x)
	   ^
  In file included from /usr/local/google/home/jlebar/code/llvm/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp:14:
  In file included from /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/queue:170:
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:1092:32: error: conflicting types for '__deque_base<_Tp, _Allocator>'
  __deque_base<_Tp, _Allocator>::__deque_base()
				 ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:964:5: note: previous declaration is here
      __deque_base()
      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:1114:32: error: conflicting types for '__deque_base<_Tp, _Allocator>'
  __deque_base<_Tp, _Allocator>::__deque_base(__deque_base&& __c)
				 ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:973:5: note: previous declaration is here
      __deque_base(__deque_base&& __c)
      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:1556:25: error: conflicting types for 'deque<_Tp, _Allocator>'
  deque<_Tp, _Allocator>::deque(deque&& __c)
			  ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:1246:5: note: previous declaration is here
      deque(deque&& __c) _NOEXCEPT_(is_nothrow_move_constructible<__base>::value);
      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:1577:25: error: conflicting types for 'operator='
  deque<_Tp, _Allocator>::operator=(deque&& __c)
			  ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:1250:12: note: previous declaration is here
      deque& operator=(deque&& __c)
	     ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:1601:25: error: conflicting types for '__move_assign'
  deque<_Tp, _Allocator>::__move_assign(deque& __c, true_type)
			  ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/deque:1455:10: note: previous declaration is here
      void __move_assign(deque& __c, true_type)
	   ^
  In file included from /usr/local/google/home/jlebar/code/llvm/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp:14:
  In file included from /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/queue:171:
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:422:33: error: conflicting types for '__vector_base<_Tp, _Allocator>'
  __vector_base<_Tp, _Allocator>::__vector_base()
				  ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:355:5: note: previous declaration is here
      __vector_base()
      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:1300:26: error: conflicting types for 'operator='
  vector<_Tp, _Allocator>::operator=(vector&& __x)
			   ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:557:13: note: previous declaration is here
      vector& operator=(vector&& __x)
	      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:1324:26: error: conflicting types for '__move_assign'
  vector<_Tp, _Allocator>::__move_assign(vector& __c, true_type)
			   ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:782:10: note: previous declaration is here
      void __move_assign(vector& __c, true_type)
	   ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:2544:27: error: conflicting types for 'vector<bool, type-parameter-0-0>'
  vector<bool, _Allocator>::vector()
			    ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:2162:5: note: previous declaration is here
      vector() _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value);
      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:2836:27: error: conflicting types for 'operator='
  vector<bool, _Allocator>::operator=(vector&& __v)
			    ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:2210:13: note: previous declaration is here
      vector& operator=(vector&& __v)
	      ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:2856:27: error: conflicting types for '__move_assign'
  vector<bool, _Allocator>::__move_assign(vector& __c, true_type)
			    ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/vector:2418:10: note: previous declaration is here
      void __move_assign(vector& __c, true_type)
	   ^
  In file included from /usr/local/google/home/jlebar/code/llvm/libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/ctor_move.pass.cpp:14:
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/queue:711:44: error: conflicting types for 'swap'
  priority_queue<_Tp, _Container, _Compare>::swap(priority_queue& __q)
					     ^
  /usr/local/google/home/jlebar/llvm/llvm/projects/libcxx/include/queue:514:10: note: previous declaration is here
      void swap(priority_queue& __q)
	   ^
  17 errors generated.
  --

  Compilation failed unexpectedly!

[mclow.lists]

error: undefined reference to '__gxx_personality_v0'

means that you're not linking to an ABI library.

That symbol comes out of libc++abi, but you can also get that from libsupc.

My build setup is similar to yours (on Mac OS X):

cd $LLVM_BUILD ; rm -rf libcxx ; mkdir libcxx ; cd libcxx 
CXX=$LLVM_BIN/clang++ cmake -DLLVM_PATH=$LLVM/llvm -DLIBCXX_CXX_ABI=libcxxabi -DLIBCXX_CXX_ABI_INCLUDE_PATHS=/usr/include $LIBCXX
make              # build libc++
make check-libcxx # run the tests

libcxx/include/cmath

615

I don't see how this can possibly be constexpr.
it calls isfinite(), which is hoisted from ::isfinite(), which comes from the C library.
Since C knows nothing about constexpr, we're stuck.

I just tried this:

constexpr float f = 3.2f;
static_assert(::isfinite(f), "" );

and got:

bug.cpp:9:19: error: static_assert expression is not an integral constant expression
static_assert(::isfinite(f), "" );
              ^~~~~~~~~~~~~

[jlebar]

Thank you, Marshall.

In D25403#572998, @mclow.lists wrote:

My build setup is similar to yours (on Mac OS X):

cd $LLVM_BUILD ; rm -rf libcxx ; mkdir libcxx ; cd libcxx 
CXX=$LLVM_BIN/clang++ cmake -DLLVM_PATH=$LLVM/llvm -DLIBCXX_CXX_ABI=libcxxabi -DLIBCXX_CXX_ABI_INCLUDE_PATHS=/usr/include $LIBCXX
make              # build libc++
make check-libcxx # run the tests

Ah, it's -DLIBCXX_CXX_ABI and -DLIBCXX_CXX_ABI_INCLUDE_PATHS that I was missing.

Is this the recommended way of building libcxx? Should I update the documentation?

Likewise, should I update the documentation to indicate that check-cxx may fail with a clang built from tip of tree, due to c++17 support being experimental? Or do you all want to change the target so that it doesn't run the c++17 tests by default? This burned about an hour of developer time yesterday, I guess because we couldn't believe that check-cxx would be intentionally broken like that, so it would be nice to have some sort of fix.

I don't see how this can possibly be constexpr.
it calls isfinite(), which is hoisted from ::isfinite(), which comes from the C library.
Since C knows nothing about constexpr, we're stuck.

Functions that call non-constexpr things can be marked constexpr; you just can't evaluate them in a constexpr context (as you demonstrated).

All I need is for the function to be marked as constexpr; I do not need the function be constexpr-evaluatable.

I think the implications of this change would be that, if you evaluated this function in a constexpr context, before the change would be a compile error when calling __libcpp_isnan, and after this change it would be a compile error at calling ::isnan. Since the function should not be called outside of libc++ anyway, I was hoping that wouldn't make a difference to anyone.

[mclow.lists]

In D25403#573104, @jlebar wrote:

Likewise, should I update the documentation to indicate that check-cxx may fail with a clang built from tip of tree, due to c++17 support being experimental? Or do you all want to change the target so that it doesn't run the c++17 tests by default? This burned about an hour of developer time yesterday, I guess because we couldn't believe that check-cxx would be intentionally broken like that, so it would be nice to have some sort of fix.

Yesterday and today is the first time in a while that clang has been seriously broken for more than an hour or so.
I'm not inclined to worry about it yet.

I don't see how this can possibly be constexpr.
it calls isfinite(), which is hoisted from ::isfinite(), which comes from the C library.
Since C knows nothing about constexpr, we're stuck.

Functions that call non-constexpr things can be marked constexpr; you just can't evaluate them in a constexpr context (as you demonstrated).

All I need is for the function to be marked as constexpr; I do not need the function be constexpr-evaluatable.

Then what's the point? How can you test if it is correct?

I think the implications of this change would be that, if you evaluated this function in a constexpr context, before the change would be a compile error when calling __libcpp_isnan, and after this change it would be a compile error at calling ::isnan. Since the function should not be called outside of libc++ anyway, I was hoping that wouldn't make a difference to anyone.

I guess I don't understand the motivation for this change.

[jlebar]

In D25403#573666, @mclow.lists wrote:

Yesterday and today is the first time in a while that clang has been seriously broken for more than an hour or so.
I'm not inclined to worry about it yet.

Oh, awesome. That sounds good to me.

The question about build configurations still stands, though -- should that be documented somewhere? In particular I could not successfully run check-cxx by following the directions I found by googling. I am volunteering to write the patch if we can agree on what the workflow should be.

I don't see how this can possibly be constexpr.
it calls isfinite(), which is hoisted from ::isfinite(), which comes from the C library.
Since C knows nothing about constexpr, we're stuck.

Functions that call non-constexpr things can be marked constexpr; you just can't evaluate them in a constexpr context (as you demonstrated).

All I need is for the function to be marked as constexpr; I do not need the function be constexpr-evaluatable.

Then what's the point? How can you test if it is correct?

D24971 tests this. I suppose we could write a test inside libcxx, but it would be kind of ugly.

I guess I don't understand the motivation for this change.

To make <complex> work in CUDA, we do the following terrible, awful, horrible, no good thing: ...well, I can just show you the code. https://github.com/llvm-project/llvm-project/blob/master/clang/lib/Headers/cuda_wrappers/complex

The significant part here is

#pragma clang force_cuda_host_device begin
#include_next <complex>
#pragma clang force_cuda_host_device end

This tells clang, everything between the two pragmas is something that we can run on the host (CPU) and device (GPU). And that works fine for libstdc++. But for libc++, marking everything inside <complex> as host+device is not enough -- we also need to mark these four functions, which are called from <complex>.

We could mark them as host device explicitly, but then we'd need checks for CUDA compilation inside of libc++, and I've been avoiding asking for that. Instead, marking these functions as constexpr works, because constexpr functions are implicitly host+device in our CUDA implementation.

[jlebar]

Friendly ping?

[hfinkel]

...

I guess I don't understand the motivation for this change.

To make <complex> work in CUDA, we do the following terrible, awful, horrible, no good thing: ...well, I can just show you the code. https://github.com/llvm-project/llvm-project/blob/master/clang/lib/Headers/cuda_wrappers/complex

The significant part here is

#pragma clang force_cuda_host_device begin
#include_next <complex>
#pragma clang force_cuda_host_device end

This tells clang, everything between the two pragmas is something that we can run on the host (CPU) and device (GPU). And that works fine for libstdc++. But for libc++, marking everything inside <complex> as host+device is not enough -- we also need to mark these four functions, which are called from <complex>.

Is this because the functions are in <cmath> instead of in <complex> are you don't want to mark all of <cmath> as host/device?

We could mark them as host device explicitly, but then we'd need checks for CUDA compilation inside of libc++, and I've been avoiding asking for that. Instead, marking these functions as constexpr works, because constexpr functions are implicitly host+device in our CUDA implementation.

[jlebar]

Is this because the functions are in <cmath> instead of in <complex> are you don't want to mark all of <cmath> as host/device?

Yes. cmath is its own beast; we need to have our own implementation of it in order to direct the std functions to the appropriate low-level device functions. (And even if this could somehow be made to work with libc++, we'd still need it for libstdc++, so doing it one way, even if it's horrible, is very likely preferable to doing it two ways.)

[hfinkel]

In D25403#580416, @jlebar wrote:

Is this because the functions are in <cmath> instead of in <complex> are you don't want to mark all of <cmath> as host/device?

Yes. cmath is its own beast; we need to have our own implementation of it in order to direct the std functions to the appropriate low-level device functions. (And even if this could somehow be made to work with libc++, we'd still need it for libstdc++, so doing it one way, even if it's horrible, is very likely preferable to doing it two ways.)

Okay. Why not fix the Clang builtins so that they're evaluatable for constant inputs in a constexpr context? Then we can do this and test the change.

[jlebar]

In D25403#580422, @hfinkel wrote:

Okay. Why not fix the Clang builtins so that they're evaluatable for constant inputs in a constexpr context? Then we can do this and test the change.

I am not sure how much value we would derive from testing specifically that these functions are constexpr-evaluatable? The thing we actually care about is whether clang can wrap the libc++ <complex> header in cuda mode, and that could break by removing these constexprs, or by adding a new function used in <complex> and not marking it constexpr, or in any number of other ways.

Conversely, if <complex> stopped relying on these functions, we wouldn't care if they stopped being constexpr.

In addition, if I understand you correctly, we wouldn't be able to test all of the functions here, only the ones that call builtins.

We have an e2e test for this in the test-suite -- it's currently only enabled for libstdc++. We haven't yet hooked this up to send out emails when it fails, but if you wanted to block this change on that, I'd totally be onboard. We also similarly have tests in the test-suite for <cmath> and <math.h>, which have similar failure modes to <complex>.

[hfinkel]

In D25403#580432, @jlebar wrote:

In D25403#580422, @hfinkel wrote:

Okay. Why not fix the Clang builtins so that they're evaluatable for constant inputs in a constexpr context? Then we can do this and test the change.

I am not sure how much value we would derive from testing specifically that these functions are constexpr-evaluatable? The thing we actually care about is whether clang can wrap the libc++ <complex> header in cuda mode, and that could break by removing these constexprs, or by adding a new function used in <complex> and not marking it constexpr, or in any number of other ways.

Conversely, if <complex> stopped relying on these functions, we wouldn't care if they stopped being constexpr.

I'm not sure about that. It seems like a useful feature for the builtins to have. Logically speaking, they should be constexpr.

In addition, if I understand you correctly, we wouldn't be able to test all of the functions here, only the ones that call builtins.

What do you mean?

We have an e2e test for this in the test-suite -- it's currently only enabled for libstdc++. We haven't yet hooked this up to send out emails when it fails, but if you wanted to block this change on that, I'd totally be onboard. We also similarly have tests in the test-suite for <cmath> and <math.h>, which have similar failure modes to <complex>.

[jlebar]

I'm not sure about that. It seems like a useful feature for the builtins to have. Logically speaking, they should be constexpr.

I agree that it's logically correct for the builtins to be constexpr-evaluatable. My point is just that doing this work and then writing a test doesn't buy us much in terms of ensuring that CUDA compilation doesn't break due to changes to libc++.

In addition, if I understand you correctly, we wouldn't be able to test all of the functions here, only the ones that call builtins.

What do you mean?

This patch adds constexpr to six function definitions, but only three of them directly call a builtin. If I understand your proposal correctly, the other three function definitions would remain not-constexpr-evaluatable, and thus untested.

[hfinkel]

In D25403#580439, @jlebar wrote:

I'm not sure about that. It seems like a useful feature for the builtins to have. Logically speaking, they should be constexpr.

I agree that it's logically correct for the builtins to be constexpr-evaluatable. My point is just that doing this work and then writing a test doesn't buy us much in terms of ensuring that CUDA compilation doesn't break due to changes to libc++.

True, it is not an e2e test, but it could insure that this is not the problem.

In addition, if I understand you correctly, we wouldn't be able to test all of the functions here, only the ones that call builtins.

What do you mean?

This patch adds constexpr to six function definitions, but only three of them directly call a builtin. If I understand your proposal correctly, the other three function definitions would remain not-constexpr-evaluatable, and thus untested.

Well, we don't know about the others because it would depend on what function was found by ADL (potentially). Given that we only use these functions right now in <complex>, and how <complex> behaves for non-builtin floating-point types is not defined, I suspect this is much less concerning.

[jlebar]

Let "CE" mean "constexpr-evaluatable".

libc++ attempts to be backwards-compatible with old versions of clang, right? Old versions of clang are going to fail, since the builtin will not be CE. Is the idea to write a test that checks that, if __builtin_isfinite is CE, then __libcpp_isfinite is also CE? AIUI you can only test CE-ness by detecting a compile failure failure.

So is the idea that we would have a shell script that tries to compile a file, checking for CE-ness of the builtins, and, if and only if that succeeds, then checks for CE-ness of __libcpp_isfinite?

[hfinkel]

Here's what I suggest:

1. We commit this patch (I think it logically makes sense; the builtins should be constexpr evaluatable, even if they're currently not)
2. We follow up by fixing Clang to make the builtins constexpr evaluatable (since I think that makes sense regardless of this)
3. We then go back and add some tests here (which would need to be predicated on the newer version of Clang)

@mclow.lists , does that make sense to you?

[hfinkel]

In D25403#580444, @jlebar wrote:

Let "CE" mean "constexpr-evaluatable".

libc++ attempts to be backwards-compatible with old versions of clang, right?

Yea, we'd need to ifdef the test for older versions of Clang. I've just summarized my overall proposal in a previous comment.

Old versions of clang are going to fail, since the builtin will not be CE. Is the idea to write a test that checks that, if __builtin_isfinite is CE, then __libcpp_isfinite is also CE? AIUI you can only test CE-ness by detecting a compile failure failure.

So is the idea that we would have a shell script that tries to compile a file, checking for CE-ness of the builtins, and, if and only if that succeeds, then checks for CE-ness of __libcpp_isfinite?

[jlebar]

So, good news -- these three builtins are already constexpr-evaluatable. :)

I'll add a test.

[jlebar]

Add a test.

[jlebar]

Hal, whadya think?

[hfinkel]

libcxx/test/libcxx/numerics/c.math/constexpr-fns.pass.cpp

24 ↗

(On Diff #77105)

I think the preferred form here is:

#if TEST_STD_VER >= 11

[jlebar]

Use TEST_STD_VER macro.

[hfinkel]

In D25403#590049, @jlebar wrote:

Use TEST_STD_VER macro.

This is fine with me; @EricWF , @mclow.lists ?

[EricWF]

libcxx/test/libcxx/numerics/c.math/constexpr-fns.pass.cpp

17 ↗

(On Diff #77271)

Does GCC offer these as contexpr? If not this needs a // XFAIL: gcc

22 ↗

(On Diff #77271)

You don't need this _LIBCPP_VERSION check here.

[jlebar]

Thanks for the review.

libcxx/test/libcxx/numerics/c.math/constexpr-fns.pass.cpp

17 ↗

(On Diff #77271)

Looks like the relevant builtins in gcc are constexpr-evaluatable: https://godbolt.org/g/GcwJba

22 ↗

(On Diff #77271)

Hm, there are lots of tests that have this, and since this test will not compile with libstdc++, if it's not appropriate here, I am confused as to when we should and shouldn't have this check. When should it be used?

[EricWF]

libcxx/test/libcxx/numerics/c.math/constexpr-fns.pass.cpp

17 ↗

(On Diff #77271)

Also instead of using TEST_STD_VER you can write // UNSUPPORTED: c++98, c++03.

22 ↗

(On Diff #77271)

Every header has a test that including it provides _LIBCPP_VERSION, but that's the only time this needs to appear.

[jlebar]

Capturing an IRC conversation:

EricWF jlebar: Did you test this patch with older Clangs w/o constexpr builtins?
jlebar EricWF, Do you mean, did I test the test, or did I test that the non-test change does what I need?
EricWF jlebar: I'm wondering if older clangs emit a never-constexpr diagnostic if the builtin isn't > constexpr.
jlebar EricWF, clang 3.3 and earlier do.
EricWF OK then we're good :-)

Thank you again for the comments; submitting now with the fixes above.

libcxx/include/cmath

static_assert((!(is_same<_A1, __result_type>::value &&
                      is_same<_A2, __result_type>::value &&
                      is_same<_A3, __result_type>::value)), "");
     return hypot((__result_type)__lcpp_x, (__result_type)__lcpp_y, (__result_type)__lcpp_z);
 }
 #endif
 
 template <class _A1>
 _LIBCPP_ALWAYS_INLINE
-typename enable_if<is_floating_point<_A1>::value, bool>::type
+_LIBCPP_CONSTEXPR typename enable_if<is_floating_point<_A1>::value, bool>::type
 __libcpp_isnan(_A1 __lcpp_x) _NOEXCEPT
 {
 #if __has_builtin(__builtin_isnan)
     return __builtin_isnan(__lcpp_x);
 #else
     return isnan(__lcpp_x);
 #endif
 }
 
 template <class _A1>
 _LIBCPP_ALWAYS_INLINE
-typename enable_if<!is_floating_point<_A1>::value, bool>::type
+_LIBCPP_CONSTEXPR typename enable_if<!is_floating_point<_A1>::value, bool>::type
 __libcpp_isnan(_A1 __lcpp_x) _NOEXCEPT
 {
     return isnan(__lcpp_x);
 }
 
 template <class _A1>
 _LIBCPP_ALWAYS_INLINE
-typename enable_if<is_floating_point<_A1>::value, bool>::type
+_LIBCPP_CONSTEXPR typename enable_if<is_floating_point<_A1>::value, bool>::type
 __libcpp_isinf(_A1 __lcpp_x) _NOEXCEPT
 {
 #if __has_builtin(__builtin_isinf)
     return __builtin_isinf(__lcpp_x);
 #else
     return isinf(__lcpp_x);
 #endif
 }
 
 template <class _A1>
 _LIBCPP_ALWAYS_INLINE
-typename enable_if<!is_floating_point<_A1>::value, bool>::type
+_LIBCPP_CONSTEXPR typename enable_if<!is_floating_point<_A1>::value, bool>::type
 __libcpp_isinf(_A1 __lcpp_x) _NOEXCEPT
 {
     return isinf(__lcpp_x);
 }
 
 template <class _A1>
 _LIBCPP_ALWAYS_INLINE
-typename enable_if<is_floating_point<_A1>::value, bool>::type
+_LIBCPP_CONSTEXPR typename enable_if<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
 }
 
 template <class _A1>
 _LIBCPP_ALWAYS_INLINE
-typename enable_if<!is_floating_point<_A1>::value, bool>::type
+_LIBCPP_CONSTEXPR typename enable_if<!is_floating_point<_A1>::value, bool>::type
 __libcpp_isfinite(_A1 __lcpp_x) _NOEXCEPT
 {
     return isfinite(__lcpp_x);
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif  // _LIBCPP_CMATH