[PATCH] D42642: [CUDA] Detect installation in PATH

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld marked 5 inline comments as done.
Hahnfeld added a subscriber: sylvestre.ledru.
Hahnfeld added inline comments.


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:96-105
+      if (llvm::ErrorOr<std::string> ptxas =
+              llvm::sys::findProgramByName("ptxas")) {
+        SmallString<256> ptxasAbsolutePath;
+        llvm::sys::fs::real_path(*ptxas, ptxasAbsolutePath);
+
+        StringRef ptxasDir = llvm::sys::path::parent_path(ptxasAbsolutePath);
+        if (llvm::sys::path::filename(ptxasDir) == "bin")
----------------
tra wrote:
> Another corner case:
> Debian scatters CUDA install all over the filesystem. To make it work with clang it has a 'shim' package which re-creates complete CUDA install using symlinks to its scattered bits. https://bugs.llvm.org/show_bug.cgi?id=35249.  If PATH includes such a shim with a symlink pointing to location somewhere else in the filesystem, this variant of the patch will not work.
> 
> I'd add another candidate derived from the path returned by find. This should cover all reasonable scenarios I can think of.
> 
> Caveat: clang on Debian already has a special case to add this shim to the list of candidates ( D40453 ), so this patch should not affect it. Still, it's possible for the similar case to happen somewhere else where we do not have any explicit workarounds in clang.
> 
> BTW, should this heuristic apply on Windows, too? IIRC cuda installer does add CUDA's bin dir to PATH.
> 
I'd rather not complicate this further. I asked on D40453 and the reply was
>>! In D40453#936114, @sylvestre.ledru wrote:
> Debian packages don't update the PATH and we are aiming at providing packages working out of the box.

So IMO if distributions opt to fully diverge from CUDA's standard directory layout they should add special cases here to make it work.

I'm not really sure regarding Windows:
1. From what I recall the `PATH` variable was somewhat dubious under Windows...
2. Does the installer actually allow to customize the installation path? If not, how likely is it that users move the directory manually?
I can't test on Windows, so I'd generally prefer if you adjust the code and add tests in a separate patch...


https://reviews.llvm.org/D42642