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

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

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). 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.

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.

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.

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.

In D55269#1319382 <https://reviews.llvm.org/D55269#1319382>, @tra wrote:

> And I believe that we did convince Debian that it's up to them to arrange their packages in a way that works with clang.
>  Granted, closer examination of the shim they've added shows that the shim is incomplete, but it's the right way to solve this problem, IMO.


I fully agree here: Adding a single candidate path for a specific distro seems worth it, and we are already doing it.

In D55269#1319382 <https://reviews.llvm.org/D55269#1319382>, @tra wrote:

> In D55269#1319319 <https://reviews.llvm.org/D55269#1319319>, @jdenny wrote:
>
> > I'm not adamant that handling --cuda-path=/usr is the right solution.  But just adding IsUbuntu() is not a full solution, so I'm looking for advice on how to proceed.
>
>
> 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).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55269