CUDA build improvements #645
Conversation
There was a problem hiding this comment.
My CMake knowledge is rusty enough at this point that I can't really parse this change. @basnijholt, how would we go about testing that this works as intended? Is it feasible to add such a test to CI, both in terms of human effort and keeping CI runs short?
| @@ -27,8 +27,10 @@ namespace qsim { | |||
| unsigned num_sim_threads, | |||
| unsigned num_state_threads, | |||
| unsigned num_dblocks | |||
| ) : ss_params{num_state_threads, num_dblocks} {} | |||
|
|
|||
| ) { | |||
There was a problem hiding this comment.
What motivates this change? AFAICT the two should behave identically.
There was a problem hiding this comment.
They should be the same but not all compilers can solve the ambiguity.
Locally on my Linux box, as well as on the conda-forge building infrastructure the build will fail with:
./pybind_interface/cuda/pybind_main_cuda.cpp(30): error: no instance of constructor "qsim::StateSpaceCUDA<FP>::Parameter::Parameter [with FP=float]" matches the argument list
basnijholt
force-pushed
the
cuda-builds
branch
2 times, most recently
from
153247b to
19a5982
Compare
|
Marking this as draft because it want to make sure it includes all patches that were required to get the |
…:Parameter [with FP=float]" matches the argument list' Full error: ./pybind_interface/cuda/pybind_main_cuda.cpp(30): error: no instance of constructor "qsim::StateSpaceCUDA<FP>::Parameter::Parameter [with FP=float]" matches the argument list
basnijholt
force-pushed
the
cuda-builds
branch
from
f50c4b6 to
cad9662
Compare
|
OK I made sure that these changes align with what happens in the conda-forge repo. These changes are based on the following patches
To give these changes some weight, @leofang helped with the CUDA builds, who works on cuQuantum at NVIDIA. |
|
@95-martin-orion I don't have the bandwidth currently to set up a CI pipeline here to build the CUDA versions of qsim. Testing it will also be hard because GitHub doesn't provide and runners with GPUs (yet). |
|
To be clear, Bas alone came up with all the patches. My involvement there was mainly to offer consultation for general CUDA + conda-forge matters, and to ensure cuQuantum's conda packages can be consumed by downstream. But these patches look fine to me. I think as long as we can ensure it builds fine after applying the patch, from the conda-forge perspective I suggest to merge this PR, so that package maintainers (Bas and I) do not have to maintain the patches indefinitely. |
This makes it possible to properly compile with CUDA support with a variety of compiler versions and modern CMake.
These changes are based on the following patches that were used to build qsim with CUDA support on conda-forge:
0001-Fix-no-instance-of-constructor-qsim-StateSpaceCUDA-F.patch0003-Set-pybind11_INCLUDE_DIRS-correctly-for-CUDA.patch0006-Set-CUDA_ARCHITECTURES-all-cmake-policy-CMP0104.patch0008-Add-support-for-additional-CMake-arguments.patch