Page MenuHomePhabricator

Add the nvidia-cuda-toolkit Debian package path to search path
ClosedPublic

Authored by sylvestre.ledru on Nov 25 2017, 1:25 AM.

Diff Detail

Repository
rC Clang

Event Timeline

Hahnfeld edited edge metadata.

The change looks good in general and is small enough and last in the candidates list to not break working setups.

What concerns me is that we have to do the same for every distribution (I know that Arch Linux for example has CUDA installed in /opt/cuda, see https://www.archlinux.org/packages/community/x86_64/cuda/). In addition many HPC systems use the modules system to install multiple versions in parallel. As such we should make the installation detection smarter:

  1. We could have an environment variable that systems can set to the appropriate directory.
  2. We could try to automatically deduce the install path: If we find ptxas in the PATH and it is in a directory bin, its parent is a good candidate to start searching.

I think 2. would work for Arch Linux which adds /opt/cuda/bin to PATH IIRC. I'm not sure about Debian though, @sylvestre.ledru? If that doesn't work, we have to add the candidate statically anyway...

Debian packages don't update the PATH and we are aiming at providing packages working out of the box.

In general, yeah, we have to do that for every distros... :/

In that case I don't see a way to be "clever", so we have to add the static candidate. I'll let @tra or @jlebar have a final look.

tra requested changes to this revision.Nov 27 2017, 3:21 PM

I'm reluctant to add a distribution-specific search path unconditionally.

We do have Distro.IsDebian() https://github.com/llvm-mirror/clang/blob/9f9177d3ef72580ca29e8844327f63d7aa1908af/include/clang/Driver/Distro.h#L112

Adding distribution-specific path for particular distribution would be fine with me.

This revision now requires changes to proceed.Nov 27 2017, 3:21 PM
sylvestre.ledru edited edge metadata.
tra added inline comments.Nov 28 2017, 1:37 PM
lib/Driver/ToolChains/Cuda.cpp
81

No need for a named temporary. if (Distro(D.getVFS).IsDebian()) should do.

Also, please add a comment describing why Debian needs a special case here and include reference to the bug report which has the details.

On a related note please add context to your patch as described here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

tra accepted this revision.Nov 28 2017, 3:24 PM
This revision is now accepted and ready to land.Nov 28 2017, 3:24 PM
jlebar removed a reviewer: jlebar.Nov 28 2017, 5:10 PM
jlebar added a subscriber: jlebar.

I defer to tra on this.

This revision was automatically updated to reflect the committed changes.