fix: support nvcc and test #2461
Conversation
| @@ -104,6 +113,16 @@ if((PYBIND11_TEST_FILES_ASYNC_I GREATER -1) AND (PYTHON_VERSION VERSION_LESS 3.5 | |||
| list(REMOVE_AT PYBIND11_TEST_FILES ${PYBIND11_TEST_FILES_ASYNC_I}) | |||
| endif() | |||
|
|
|||
| # Skip tests for CUDA check: | |||
| # /pybind11/tests/test_constants_and_functions.cpp(125): | |||
| # error: incompatible exception specifications | |||
There was a problem hiding this comment.
What does "incompatible exception specification" mean?
There was a problem hiding this comment.
That's printed by NVCC when running. It doesn't like either noexcept(false) or throw(), I believe, but the error message line is off (I think), so it's hard to tell what it is unhappy with.
tests/test_copy_move.cpp
Outdated
| @@ -175,14 +175,17 @@ TEST_SUBMODULE(copy_move_policies, m) { | |||
| m.attr("has_optional") = false; | |||
| #endif | |||
|
|
|||
| // #70 compilation issue if operator new is not public | |||
| // #70 compilation issue if operator new is not public - body added but not needed on most compilers | |||
There was a problem hiding this comment.
This comment deserves a longer explanation. Which compilers behave oddly, is the first thing that popped into my mind when reading this code.
I know we know which ones today, but will we still remember in a year?
There was a problem hiding this comment.
MSVC and nvcc don't like having a definition with no body, even if it is never used. Having a body is better than manually trying to hide the warning, IMO, and when you start trying to hide the warning from multiple compilers, that explodes. (will add something like this to the note)
There was a problem hiding this comment.
I could probably move this out of the test definition, then the compilers would be okay with having a declaration without a definition. What they are complaining about is that these are local, so there's never a way to add the method body.
There was a problem hiding this comment.
I added the longer note; let me know if you think it's a good idea to move it up and out of the test body, so compilers will not complain about the missing definition - then we should be able to leave it empty.
| @@ -1483,7 +1483,14 @@ struct vectorize_arg { | |||
|
|
|||
| template <typename Func, typename Return, typename... Args> | |||
| struct vectorize_helper { | |||
|
|
|||
| // NVCC for some reason breaks if NVectorized is private | |||
There was a problem hiding this comment.
It claims NVectorize is private in this context and quits.
[ 73%] Building CUDA object tests/CMakeFiles/pybind11_tests.dir/test_opaque_types.cpp.o
cd /build/tests && /usr/local/cuda/bin/nvcc -Dpybind11_tests_EXPORTS -I/pybind11/include -isystem=/usr/include/python3.8 -O1 -DNDEBUG -Xcompiler=-fPIC -Xcompiler=-fvisibility=hidden -Werror all-warnings -std=c++11 -x cu -c /pybind11/tests/test_opaque_types.cpp -o CMakeFiles/pybind11_tests.dir/test_opaque_types.cpp.o
/pybind11/tests/test_numpy_vectorize.cpp: In lambda function:
/pybind11/tests/test_numpy_vectorize.cpp:86:6: error: 'constexpr const size_t pybind11::detail::vectorize_helper<double (*)(int, float, double), double, int, float, double>::NVectorized' is private within this context
86 | std::array<py::buffer_info, 3> buffers {{ arg1.request(), arg2.request(), arg3.request() }};
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/pybind11/include/pybind11/numpy.h:1490:25: note: declared private here
1490 | static constexpr size_t NVectorized = constexpr_sum(vectorize_arg<Args>::vectorize...);
|
By the way, you can run: docker run --rm -it -v $PWD:/pybind11 nvidia/cuda:11.0-devel-ubuntu20.04 bash
apt-get update && DEBIAN_FRONTEND="noninteractive" apt-get install -y cmake python3-dev python3-pytest
cmake -S pybind11 -B build -DPYBIND11_CUDA_TESTS=ON -DPYBIND11_WERROR=ON
cmake --build build -v -j2
cmake --build build --target pytestTo do the testing locally. |
| @@ -1036,10 +1057,8 @@ struct type_caster<T, enable_if_t<std::is_arithmetic<T>::value && !is_std_char_t | |||
|
|
|||
| bool py_err = py_value == (py_type) -1 && PyErr_Occurred(); | |||
|
|
|||
| // Protect std::numeric_limits::min/max with parentheses | |||
There was a problem hiding this comment.
Not a big fan of this change, isn't there some way to #pragma be quiet NVCC?
There was a problem hiding this comment.
It's poorly documented, but yes, I think more recent NVCC versions support suppression if you can find the right code. But this will actually generate simpler/faster runtime code with 1-2 fewer checks, if this happens fairly often. We can move the sizeof check to compile time, too, I believe.
I also think the logic may be flawed; it might be good to rework with new logic. Also I'm surprised it's showing up with some very odd types; things like pointers were static_cast to an integer is not allowed (which is why it's using C-style casts); I guess this is used to covert pointer addresses to Python ints?
There was a problem hiding this comment.
Check the new version out! Simpler logic (the tests here are pretty comprehensive, don't ask me how I know that 😊), with no more pointless comparisons.
|
Fine with all of these changes, except the range check that has become very complex with the extra SFINAE. |
|
Okay, thanks! Going in, will rebase the PGI PR on it soon, since they collide a bit on a couple of lines. |
Adding a CI test for NVCC, along with fixes and if statements. Everyone is welcome to suggest better fixes, especially for deactivated tests!
invalid_convert_integral) into compile time to fix warning (pointless comparison with 0) and provide better performance (will add comments and fix my one existing comment if that's the way we go)vectorize_helpercomplaining about private usage internally*
test_constants_and_functions.cppskipped due to "incompatible exception specifications"void *operator new(size_t bytes)(fixes warning on Windows, too)NonCopyablenot supported.