Get device capability #734
Conversation
dfalbel
added
the
lantern
There was a problem hiding this comment.
Looks great @rdinnager ! Thanks!! The comments are minor things that we need to tweak in order to also be able to build without CUDA.
__CUDACC__ is supposed to tell code is being compiled by nvcc or not according to this SO post.
The full CI failure will be solved in the next commit as GH actions we build lantern manually.
Let me know if you have additional questions.
| @@ -4,6 +4,7 @@ | |||
|
|
|||
| #include "lantern/lantern.h" | |||
|
|
|||
| #include <ATen/cuda/CUDAContext.h> | |||
There was a problem hiding this comment.
You will need to wrap this around something like #ifdef __CUDACC__ to not include this header when CUDA is not available.
There was a problem hiding this comment.
Yes, good point. I only tested this on cuda compiled torch previously, and I didn't think about this. This makes perfect sense. Done.
lantern/src/Cuda.cpp
Outdated
| void* _lantern_cuda_get_device_capability(int64_t device) | ||
| { | ||
| LANTERN_FUNCTION_START | ||
| cudaDeviceProp * devprop = at::cuda::getDeviceProperties(device); |
There was a problem hiding this comment.
Simlarly you can wrap the body of the function in #ifdef __CUDACC__ and just raise a std::runtime_error() if that is not defined.
There was a problem hiding this comment.
Okay, that is done too.
There was a problem hiding this comment.
Okay, __CUDACC__ does not seem to be defined even when compiling with ENV{CUDA} defined on my local computer (Windows). This: https://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html#nvcc-identification-macro suggests __CUDACC__ is only defined when compiling .cu files, but __NVCC__ should be defined for .cpp files as well. Nevertheless testing for __NVCC__ does not seem to work either on my local computer. Some conversation here: boostorg/signals2#53 suggests that nvcc by default uses VC++ for .cpp files anyway and so __NVCC__ might not be set in that case. I'm not sure if this is just a windows issue. I am going to make a commit to run CI, just to see whether this works on different platforms. We may have to set a custom define in the CMakeLists.txt based on ENV{CUDA} and test for that instead.
There was a problem hiding this comment.
Ohh ! It actually makes sense as it seems that cuda_add_library will only compile .cu files with nvcc.
It seems that we could add: set_source_files_properties(test.cpp PROPERTIES LANGUAGE CUDA) as suggested here when CUDA is enabled - somewhere in here: https://github.com/mlverse/torch/blob/master/lantern/CMakeLists.txt#L133
There was a problem hiding this comment.
Okay, set_source_files_properties(src/Cuda.cpp PROPERTIES LANGUAGE CUDA) at first just caused Cuda.cpp to not get compiled at all. After adding enable_language(CUDA) to the CMakeLists.txt, nvcc was used to compile Cuda.cpp but then there was a compilation error. Likely something to do with incorrect flags or some such. Seemed like more trouble than it was worth, so I ended up just setting a definition using set_source_files_properties(src/Cuda.cpp PROPERTIES COMPILE_DEFINITIONS __NVCC__). This solution works, at least on my machine. Is this an acceptable solution do you think? I could change the flag to something other than __NVCC__ too, in case that makes it easier to remember it is something not being set by the compiler, but manually?
There was a problem hiding this comment.
This solution looks great @rdinnager ! Thanks! Let's leave __NVCC__, we can change it later if something comes up.
…n whether compilation is with CUDA.
|
Well, looks like all CI failed because it still says that |
|
Oh, do I need to do this: ab03e81 ? |
Oops, quoted out the wrong folder. Will try again after attempting the |
This PR adds a
cuda_get_device_properties(device)function which returns the major and minor cuda capability ofdevice(specified as an integer), which fixes #729. This turned out to be more difficult than I imagined (with my limited C++ abilities), and I had to dig around in thetorchsource code substantially to find examples that helped me figure out what to do. For that reason, this definitely needs a code review, because though what I've done does seem to work, I don't 100% understand everything I did (though I feel like I'm inching closer to full understanding), and so it might not be strictly correct or optimal.Also, apologies for the extra commits. I accidentally included changes from my previous PR originally. So I made an extra commit to reverse those changes so this doesn't overlap with the previous PR. Hopefully this isn't too annoying. If it is let me know and I can try and squash my commits or something..