New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cuda
: CMake add CUDA first class language support
#23021
cuda
: CMake add CUDA first class language support
#23021
Conversation
a8cb3ed
to
77dfe30
Compare
1b3e72e
to
6a63cd0
Compare
6a63cd0
to
8b87ef2
Compare
8b87ef2
to
bf88303
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
cmake/OpenCVDetectCUDA.cmake
Outdated
@@ -1,3 +1,50 @@ | |||
if(ENABLE_CUDA_FIRST_CLASS_LANGUAGE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 375 changes in this file which size is 577 lines on 4.x branch (more that 50%).
To avoid mess and/or breaking of existed legacy code my proposal is creating a copy of this file, cleanup dead code, and specify code for the new CUDA support.
|
||
if(NOT CUDA_FOUND) | ||
find_host_package(CUDA ${OpenCV_CUDA_VERSION} EXACT REQUIRED) | ||
set(ENABLE_CUDA_FIRST_CLASS_LANGUAGE "@ENABLE_CUDA_FIRST_CLASS_LANGUAGE@") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same issue: 70 changes for 48 lines.
Please create dedicated file.
c978566
to
07e9f8d
Compare
Changes applied and tested on both Windows and Ubuntu (dynamic and static) although I am sure I haven't tested every conceivable combination of build parameters which might fail. |
cuda
: CMake add CUDA first class language support
@asmorkalov what does this PR need for this to be part of 4.9.0? |
@alalek, @asmorkalov I have just noticed that in the latest version of CMake (3.27) FindCUDA will be removed unless the below policy flag is used.
Thats fine as OpenCV has a fallback version but this will probably mean that bugs like this one #23280 won't be fixed in future. What needs to be changed for this to be merged? |
ff100dd
to
3d1208d
Compare
3d1208d
to
aff1fb0
Compare
aff1fb0
to
14bb9bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested build procedure on Linux with CUDA 12 and 11. Overall looks good to me. One question with keeping CUDA location guessing need to be discussed.
I'm trying to apply these patches to 4.8.1 to build a conan package. I hade to make some minor modification to get it to apply but I think I did it correctly but I guess maybe not. I'm having problems with the the auto detection of CUDA architectures during configure, is it supposed to work? The variable ${CUDA_NVCC_EXECUTABLE} does not seem to be defined. |
80a6d5f
to
a20f6b0
Compare
@danielnilsson9 sorry I've just notice that I broke that when I unified everything into one utils file last month. I've pushed a fix which I am still testing but feel free to try it in the meantime. |
@mshabunin Is there any harm in including it with the below status reports?
CI has passed but I need to update some of the status messages for Linux am I OK to do so? |
I think it's OK to include such messages. I'd also add hints regarding adding nvcc location to the |
@cudawarped Thank you for the quick fix! I got a bit further now, I can build if my host compiler is in PATH but that should not be required I think, since CMake discoveres the compiler. It think it needs to be passed as argument to nvcc (-ccbin). |
|
||
if(OPENCV_CUDA_DETECTION_NVCC_FLAGS MATCHES "-ccbin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code needs to be kept to auto set host compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added back in, in the latest commit, can you take a look if you have chance?
a20f6b0
to
b725b3c
Compare
@r2d3 Do you have any more comments regarding this PR? |
I'm back at this again but this time trying to build opencv with CUDA (using cmake/ninja) and clang 16 on ubuntu 22.04 LTS. I have set My conan packaged cuda toolkit/bin is in path and env CUDA_PATH point to root of toolkit. For some reason
fails to discover cuda compiler but if I uncomment these two lines and do enable_language(CUDA) directly instead it works, I need to investigate that further. EDIT: You must also set CMAKE_CUDA_COMPILER when setting CMAKE_CUDA_HOST_COMPILER for check language to work (https://gitlab.kitware.com/cmake/cmake/-/issues/25093#note_1392691). When I get past that if fails at ocv_filter_available_architecture. If I pass "-ccbin clang++" using OPENCV_CUDA_DETECTION_NVCC_FLAGS it will fail anyway since ccbin is then passed twice. I will try building from source from your branch tomorrow and post logs if you want. Currently building using an older patch where your latest changes have not ben added.. |
I'm new to clang so forgive me if I am missing something. How are you instructing cmake to use clang as the host compiler? If I set
Does this work if you set |
No, not really, I don't have gcc installed at all on my system (except libstdc++), only clang and it is set as default compiler on system so CMake finds it automatically (linked in /usr/bin/c++, /usr/bin/cc and ENV $CC and $CXX are set to point to clang). If you have gcc installed and on your system nvcc will automatically use if you do not pass -ccbin clang++, so that may be why it appears to work for you even though clang was not used when the architecture tests where done and they only worked because GCC was present on system. From https://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html:
I have not tried that yet, will do. |
My mistake.
Thank you for the link on my system the following compilation flags work when gcc is not found
@danielnilsson4 I think that this configuration should be passed by the user if they want to use clang and we can hint at this when CUDA is not found, e.g.
What do you think? |
Looks good, I think that is the correct approach to take. I looked at your latest code, seems like you had already fixed the issue I had in ocv_filter_available_architecture, I had not adjusted my patch with yours, sorry about that. I think it is all ok then, I had: Where you must have removed -ccbin=${HOST_COMPILER_BIN_DIR} at some point which I had missed. |
b725b3c
to
64066c5
Compare
64066c5
to
7d681cf
Compare
Rebased and fixed conflicts with #24744 |
…reak existing behaviour.
@cudawarped I disabled |
Tested manually with:
|
macro(ocv_set_cuda_detection_nvcc_flags cuda_host_compiler_var) | ||
if(OPENCV_CUDA_DETECTION_NVCC_FLAGS MATCHES "-ccbin") | ||
# already specified by user | ||
elseif(${cuda_host_compiler_var} AND EXISTS "${${cuda_host_compiler_var}}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upgraded our conan packaged solution to OpenCV 4.9.0. This EXISTS check requires that the full path to clang++ is specified, it's not possible to just set CMAKE_CUDA_HOST_COMPILER=clang++. A bit annoying but not a huge deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we might call find_program
here? Or somewhere earlier.
This PR adds CUDA first class language support which has been avaliable since CMake 3.8. Currently OpenCV uses the depreciated (since CMake 3.10) FindCUDA module.
This new feature is added automatically when OpenCV is built against CMake >= 3.18 (when the FindCUDAToolkit results variable
CUDAToolkit_LIBRARY_ROOT
was added). It can be disabled by passing-DENABLE_CUDA_FIRST_CLASS_LANGUAGE=OFF
to CMake.All projects which use OpenCV targets built with
-DENABLE_CUDA_FIRST_CLASS_LANGUAGE=ON
need to use CMake >=3.18 due to the new imported targets provided by FindCUDAToolkit.Fixes #23422, fixes #23659, fixes #24034, fixes #24177, fixes #24195, fixes opencv/opencv_contrib#3143, fixes opencv/opencv_contrib#3414
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.