[PATCH] D55269: [CUDA][OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu

Wed Dec 5 09:44:15 PST 2018

tra added a comment.

In D55269#1320207 <https://reviews.llvm.org/D55269#1320207>, @Hahnfeld wrote:

> I think there are some misunderstandings here, or at least I understand things differently than @tra is describing them: AFAICS this change is NOT about replacing `nvcc` by `clang` in any CMake project (be that the OpenMP runtime or any other project outside of LLVM).

I was not talking about replacing nvcc with clang, but rather just pointed that CUDA support in cmake is geared towards nvcc only and may not always give the right answer for clang.

> The scope of the second issue is that there is a CMake project (namely `openmp`) which uses `FindCuda.cmake` to detect the CUDA installation and passes the resulting path to `--cuda-path`. I wrote that detection and I still think it's correct because that is a way to force Clang to use a given CUDA install location. If `FindCuda.cmake` returns a wrong path (`/usr` instead of `/usr/lib/cuda`) or the shim package behind `/usr/lib/cuda` is incomplete, then this needs to be fixed and not workarounded in Clang.

We are in agreement here.  I see no problem with explicitly specifying CUDA path with --cuda-path and indeed, do think that CUDA detection in Cmake currently provides the path that does not work for clang. It's not exactly invalid as it is a valid path if one were to use nvcc, but that relies on some additional settings debian/ubuntu sets in the environment that only work for nvcc.

> That said, I think it's the right way to add `isUbuntu()` to the candidate detection that we already have for Debian. Simply because the compiler should just get most of the simple cases right automatically.

That part I'm also fine with as Ubuntu uses exactly the same shim Debian has implemented and it makes sense for Clang to benefit from it on Ubuntu.

> 
> 
> In D55269#1319109 <https://reviews.llvm.org/D55269#1319109>, @jdenny wrote:
> 
>> [...]
>>
>> In D55269#1318901 <https://reviews.llvm.org/D55269#1318901>, @tra wrote:
>>
>> > --cuda-path=/usr was never supposed to work -- /usr is *not* the root of the CUDA SDK.
>>
>>
>> /usr/lib/cuda/bin/nvcc doesn't exist, so that's probably why FindCUDA.cmake finds /usr/bin/nvcc (also installed by nvidia-cuda-toolkit).  Is it fair then to say that /usr/lib/cuda isn't the root either?
>>
>> [...]
>>
>> It seems that nvidia-cuda-toolkit still isn't installing a complete CUDA install in one location.  Even if it installed it all to /usr/lib/cuda, FindCUDA.cmake would probably still see /usr/bin/nvcc and assume /usr is the CUDA install root.
> 
> 
> I think this needs to be fixed then: The shim package should provide links to all necessary things and CMake must be prepared to deal with it. IMO we shouldn't workaround broken build system detection in the compiler.

That's exactly what I proposed to Debian folks https://bugs.llvm.org/show_bug.cgi?id=26966#c6 and I was under impression that that's what they did. It appears that they only created an empty directory structure with version.txt in it. At least that's what I see when I unpack nvidia-cuda-toolkit_9.1.85-3ubuntu1_amd64.deb. Perhaps they do something extra in the install script, but I didn't find anything obvious in the deb itself.

> 
> 
> In D55269#1319116 <https://reviews.llvm.org/D55269#1319116>, @jdenny wrote:
> 
>> By the way, nvidia-cuda-toolkit does install the following, but there's no nvvm directory as clang currently expects:
> 
> 
> Then again the distro's packaging is broken and needs to be adjusted.

Perhaps we can build a shim package ourselves and distribute it along with the clang. This would decouple usability of clang from the Ubuntu/Debian release process and would make it possible to make clang work with CUDA on older debian/Ubuntu versions.

>> Let's start with fixing OpenMP's cmake files. Once it no longer insists on specifying --cuda-path=/usr, and isUbuntu is in place, what is the remaining failure that you see?
> 
> I disagree here: It's not OpenMP but CMake that's detecting the wrong path here. This is the place to fix it once and for all (and possibly get the shim package right in that process).

It's somewhat orthogonal, IMO. I agree that it's not OpenMP's problem. Cmake will fix it at some point in future, but, presumably, we want OpenMP buildable *now*. Adding a temporary workaround for something that Cmake can't handle now would make building OpenMP with clang somewhat easier which was the end goal of this patch. In the end it's up to OpenMP maintainers what they would want to do.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55269/new/

https://reviews.llvm.org/D55269