[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca created this revision.
mojca added a reviewer: carlosgalvezp.
mojca added a project: clang.
Herald added subscribers: usaxena95, kadircet, yaxunl.
mojca requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.

I've been trying to address the following issue in `clangd` for Visual Studio 
Code trying to access CUDA:
https://github.com/clangd/clangd/issues/793

This patch alone is not yet sufficient for a fully functional clangd, but it 
gets rid of the error message saying

  Cannot find CUDA installation. Provide its path via --cuda-path, or pass 
-nocudainc to build without CUDA includes.

in case clangd and the code is located on the `C:` drive.

In my case I had both `clangd` (which I needed for debugging purposes) and the 
source code on `D:`. At this point:

  c++
  Candidates.emplace_back(D.SysRoot + "/Program Files/NVIDIA GPU Computing 
Toolkit/CUDA/v" + Ver);

the `D.SysRoot` seems to be empty. Then, during the call to

  c++
   if (InstallPath.empty() || !FS.exists(InstallPath))

after `sys::fs::make_absolute(WD->Resolved, Storage);` (in 
`VirtualFileSystem.cpp`) the `Storage` expands to `D:/Program Files/NVIDIA GPU 
Computing Toolkit/CUDA/v11.4` which isn't found either.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114326

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -127,7 +127,9 @@
   SmallVector Candidates;
 
   // In decreasing order so we prefer newer versions to older versions.
-  std::initializer_list Versions = {"8.0", "7.5", "7.0"};
+  std::initializer_list Versions = {
+  "11.5", "11.4", "11.3", "11.2", "11.1", "11.0", "10.2", "10.1",
+  "10.0", "9.2",  "9.1",  "9.0",  "8.0",  "7.5",  "7.0"};
   auto &FS = D.getVFS();
 
   if (Args.hasArg(clang::driver::options::OPT_cuda_path_EQ)) {


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -127,7 +127,9 @@
   SmallVector Candidates;
 
   // In decreasing order so we prefer newer versions to older versions.
-  std::initializer_list Versions = {"8.0", "7.5", "7.0"};
+  std::initializer_list Versions = {
+  "11.5", "11.4", "11.3", "11.2", "11.1", "11.0", "10.2", "10.1",
+  "10.0", "9.2",  "9.1",  "9.0",  "8.0",  "7.5",  "7.0"};
   auto &FS = D.getVFS();
 
   if (Args.hasArg(clang::driver::options::OPT_cuda_path_EQ)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

Well, even after this patch neither `clang` nor `clangd` work correctly for me 
(I need some patches in llvm/clang sources, there are some issues with 
Microsoft's libraries; I wasn't able to make the linker work even after that), 
and CMake doesn't fully support CUDA + Clang on Windows either.
This is just the first baby step, there's probably still a relatively long way 
to go before anyone can actually use this chunk of the code out of the box.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

And thanks to Carlos for accepting the patch.

(In case it's not a super demanding task, I would be willing to invest a bit of 
time towards making CUDA + Clang on Windows work better, but it would help to 
have "a supervisor" I could turn to when I get stuck or when I have questions.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:131
+  std::initializer_list Versions = {
+  "11.5", "11.4", "11.3", "11.2", "11.1", "11.0", "10.2", "10.1",
+  "10.0", "9.2",  "9.1",  "9.0",  "8.0",  "7.5",  "7.0"};

kadircet wrote:
> looks like the list is getting big and hard to maintain. considering that 
> this is done only once per compiler invocation (and we check for existence of 
> directories down in the loop anyway). what about throwing in an extra 
> directory listing to base-directories mentioned down below and populate 
> `Candidates` while preserving the newest-version-first order?
I totally agree with the sentiment, and that was my initial thought as well, 
but with zero experience I was too scared to make any more significant changes.

I can try to come up with a new patch (that doesn't need further maintenance 
whenever a new CUDA version gets released) if that's what you are suggesting. I 
would nevertheless merge this one, and prepare a new more advanced patch 
separately, but that's finally your call.

What's your suggestion about D.SysRoot + "Program Files/..."? At the time when 
this function gets called it looks like D.SysRoot is empty (or at least my 
debugger says so) and in my case it resolves to D: while the CUDA support is 
installed under C:.

Is there any special LLVM-specific/preferrable way to iterate through 
directories?

(What I also miss a bit in the whole process in an option to simply say "I want 
CUDA 11.1" without the need to explicitly spell out the full path.)

If you provide me give some general guidelines, I'll prepare another, hopefully 
more future-proof patch.

(Side note: I'm not sure if I'm calling clang-format correctly, but if I call 
it, it keeps reformatting the rest of this file.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:131
+  std::initializer_list Versions = {
+  "11.5", "11.4", "11.3", "11.2", "11.1", "11.0", "10.2", "10.1",
+  "10.0", "9.2",  "9.1",  "9.0",  "8.0",  "7.5",  "7.0"};

tra wrote:
> mojca wrote:
> > kadircet wrote:
> > > looks like the list is getting big and hard to maintain. considering that 
> > > this is done only once per compiler invocation (and we check for 
> > > existence of directories down in the loop anyway). what about throwing in 
> > > an extra directory listing to base-directories mentioned down below and 
> > > populate `Candidates` while preserving the newest-version-first order?
> > I totally agree with the sentiment, and that was my initial thought as 
> > well, but with zero experience I was too scared to make any more 
> > significant changes.
> > 
> > I can try to come up with a new patch (that doesn't need further 
> > maintenance whenever a new CUDA version gets released) if that's what you 
> > are suggesting. I would nevertheless merge this one, and prepare a new more 
> > advanced patch separately, but that's finally your call.
> > 
> > What's your suggestion about D.SysRoot + "Program Files/..."? At the time 
> > when this function gets called it looks like D.SysRoot is empty (or at 
> > least my debugger says so) and in my case it resolves to D: while the CUDA 
> > support is installed under C:.
> > 
> > Is there any special LLVM-specific/preferrable way to iterate through 
> > directories?
> > 
> > (What I also miss a bit in the whole process in an option to simply say "I 
> > want CUDA 11.1" without the need to explicitly spell out the full path.)
> > 
> > If you provide me give some general guidelines, I'll prepare another, 
> > hopefully more future-proof patch.
> > 
> > (Side note: I'm not sure if I'm calling clang-format correctly, but if I 
> > call it, it keeps reformatting the rest of this file.)
> This whole list may no longer be particularly useful. The most common use 
> case on Linux, AFAICT, is to install only one CUDA version using 
> system-provided package manager.
> E.g. https://packages.ubuntu.com/focal/amd64/nvidia-cuda-toolkit/filelist
> 
> TBH, I'm tempted to limit autodetection to only that one system-default 
> version and require user to use --cuda-path if they need something else.
On Windows this is certainly not the case. Unless the installation is changed 
manually, one always gets the new version installed into a new directory.

I really do need multiple versions on Windows (and the ability to pick an older 
one) if I want to compile a binary that works on someone else's computer (if I 
compile against the latest CUDA, users need "the latest" drivers that may 
sometimes not even be available for their machine).

(That said, at least when using CMake, the selection needs to be done by CMake 
anyway, and maybe CMake could be forced to specify the correct flag 
automatically.)

So even if the functionality gets removed from non-Windows platforms, it would 
be really nice to keep it for Windows.

Now, there are three "conflicting" feedbacks/suggestions above. I can try to 
improve support, but it would be really helpful to reach the consensus of what 
needs to be done before coding it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:131
+  std::initializer_list Versions = {
+  "11.5", "11.4", "11.3", "11.2", "11.1", "11.0", "10.2", "10.1",
+  "10.0", "9.2",  "9.1",  "9.0",  "8.0",  "7.5",  "7.0"};

carlosgalvezp wrote:
> mojca wrote:
> > tra wrote:
> > > mojca wrote:
> > > > kadircet wrote:
> > > > > looks like the list is getting big and hard to maintain. considering 
> > > > > that this is done only once per compiler invocation (and we check for 
> > > > > existence of directories down in the loop anyway). what about 
> > > > > throwing in an extra directory listing to base-directories mentioned 
> > > > > down below and populate `Candidates` while preserving the 
> > > > > newest-version-first order?
> > > > I totally agree with the sentiment, and that was my initial thought as 
> > > > well, but with zero experience I was too scared to make any more 
> > > > significant changes.
> > > > 
> > > > I can try to come up with a new patch (that doesn't need further 
> > > > maintenance whenever a new CUDA version gets released) if that's what 
> > > > you are suggesting. I would nevertheless merge this one, and prepare a 
> > > > new more advanced patch separately, but that's finally your call.
> > > > 
> > > > What's your suggestion about D.SysRoot + "Program Files/..."? At the 
> > > > time when this function gets called it looks like D.SysRoot is empty 
> > > > (or at least my debugger says so) and in my case it resolves to D: 
> > > > while the CUDA support is installed under C:.
> > > > 
> > > > Is there any special LLVM-specific/preferrable way to iterate through 
> > > > directories?
> > > > 
> > > > (What I also miss a bit in the whole process in an option to simply say 
> > > > "I want CUDA 11.1" without the need to explicitly spell out the full 
> > > > path.)
> > > > 
> > > > If you provide me give some general guidelines, I'll prepare another, 
> > > > hopefully more future-proof patch.
> > > > 
> > > > (Side note: I'm not sure if I'm calling clang-format correctly, but if 
> > > > I call it, it keeps reformatting the rest of this file.)
> > > This whole list may no longer be particularly useful. The most common use 
> > > case on Linux, AFAICT, is to install only one CUDA version using 
> > > system-provided package manager.
> > > E.g. https://packages.ubuntu.com/focal/amd64/nvidia-cuda-toolkit/filelist
> > > 
> > > TBH, I'm tempted to limit autodetection to only that one system-default 
> > > version and require user to use --cuda-path if they need something else.
> > On Windows this is certainly not the case. Unless the installation is 
> > changed manually, one always gets the new version installed into a new 
> > directory.
> > 
> > I really do need multiple versions on Windows (and the ability to pick an 
> > older one) if I want to compile a binary that works on someone else's 
> > computer (if I compile against the latest CUDA, users need "the latest" 
> > drivers that may sometimes not even be available for their machine).
> > 
> > (That said, at least when using CMake, the selection needs to be done by 
> > CMake anyway, and maybe CMake could be forced to specify the correct flag 
> > automatically.)
> > 
> > So even if the functionality gets removed from non-Windows platforms, it 
> > would be really nice to keep it for Windows.
> > 
> > Now, there are three "conflicting" feedbacks/suggestions above. I can try 
> > to improve support, but it would be really helpful to reach the consensus 
> > of what needs to be done before coding it.
> > one always gets the new version installed into a new directory.
> A similar thing happens on Linux.
> 
> > users need "the latest" drivers
> Since CUDA 10.2, there's some "[[ 
> https://docs.nvidia.com/deploy/cuda-compatibility/ | compatibility mode ]]" 
> that allows to run newer CUDA on older drivers. As long as you are not using 
> the latest features, of course.
> 
> I'm personally all up for removing redundancy and duplication. 
I'm following https://docs.nvidia.com/cuda/wsl-user-guide/index.html right now 
and the NVIDIA's "official packages" for Ubuntu get installed under 
`/usr/local/cuda-11.x`.

That sounds significant enough to me to argue against the removal of versioned 
folders from search.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:131
+  std::initializer_list Versions = {
+  "11.5", "11.4", "11.3", "11.2", "11.1", "11.0", "10.2", "10.1",
+  "10.0", "9.2",  "9.1",  "9.0",  "8.0",  "7.5",  "7.0"};

tra wrote:
> tra wrote:
> > mojca wrote:
> > > carlosgalvezp wrote:
> > > > mojca wrote:
> > > > > tra wrote:
> > > > > > tra wrote:
> > > > > > > mojca wrote:
> > > > > > > > kadircet wrote:
> > > > > > > > > looks like the list is getting big and hard to maintain. 
> > > > > > > > > considering that this is done only once per compiler 
> > > > > > > > > invocation (and we check for existence of directories down in 
> > > > > > > > > the loop anyway). what about throwing in an extra directory 
> > > > > > > > > listing to base-directories mentioned down below and populate 
> > > > > > > > > `Candidates` while preserving the newest-version-first order?
> > > > > > > > I totally agree with the sentiment, and that was my initial 
> > > > > > > > thought as well, but with zero experience I was too scared to 
> > > > > > > > make any more significant changes.
> > > > > > > > 
> > > > > > > > I can try to come up with a new patch (that doesn't need 
> > > > > > > > further maintenance whenever a new CUDA version gets released) 
> > > > > > > > if that's what you are suggesting. I would nevertheless merge 
> > > > > > > > this one, and prepare a new more advanced patch separately, but 
> > > > > > > > that's finally your call.
> > > > > > > > 
> > > > > > > > What's your suggestion about D.SysRoot + "Program Files/..."? 
> > > > > > > > At the time when this function gets called it looks like 
> > > > > > > > D.SysRoot is empty (or at least my debugger says so) and in my 
> > > > > > > > case it resolves to D: while the CUDA support is installed 
> > > > > > > > under C:.
> > > > > > > > 
> > > > > > > > Is there any special LLVM-specific/preferrable way to iterate 
> > > > > > > > through directories?
> > > > > > > > 
> > > > > > > > (What I also miss a bit in the whole process in an option to 
> > > > > > > > simply say "I want CUDA 11.1" without the need to explicitly 
> > > > > > > > spell out the full path.)
> > > > > > > > 
> > > > > > > > If you provide me give some general guidelines, I'll prepare 
> > > > > > > > another, hopefully more future-proof patch.
> > > > > > > > 
> > > > > > > > (Side note: I'm not sure if I'm calling clang-format correctly, 
> > > > > > > > but if I call it, it keeps reformatting the rest of this file.)
> > > > > > > This whole list may no longer be particularly useful. The most 
> > > > > > > common use case on Linux, AFAICT, is to install only one CUDA 
> > > > > > > version using system-provided package manager.
> > > > > > > E.g. 
> > > > > > > https://packages.ubuntu.com/focal/amd64/nvidia-cuda-toolkit/filelist
> > > > > > > 
> > > > > > > TBH, I'm tempted to limit autodetection to only that one 
> > > > > > > system-default version and require user to use --cuda-path if 
> > > > > > > they need something else.
> > > > > > I think on windows (I mean the windows environment itself, not 
> > > > > > WSL), CUDA installer sets an environment variable which could be 
> > > > > > used to detect the default CUDA version, so it may warrant a 
> > > > > > windows-specific way to find it. 
> > > > > On Windows this is certainly not the case. Unless the installation is 
> > > > > changed manually, one always gets the new version installed into a 
> > > > > new directory.
> > > > > 
> > > > > I really do need multiple versions on Windows (and the ability to 
> > > > > pick an older one) if I want to compile a binary that works on 
> > > > > someone else's computer (if I compile against the latest CUDA, users 
> > > > > need "the latest" drivers that may sometimes not even be available 
> > > > > for their machine).
> > > > > 
> > > > > (That said, at least when using CMake, the selection needs to be done 
> > > > > by CMake anyway, and maybe CMake could be forced to specify the 
> > > > > correct flag automatically.)
> > > > > 
> > > > > So even if the functionality gets removed from non-Windows platforms, 
> > > > > it would be really nice to keep it for Windows.
> > > > > 
> > > > > Now, there are three "conflicting" feedbacks/suggestions above. I can 
> > > > > try to improve support, but it would be really helpful to reach the 
> > > > > consensus of what needs to be done before coding it.
> > > > > one always gets the new version installed into a new directory.
> > > > A similar thing happens on Linux.
> > > > 
> > > > > users need "the latest" drivers
> > > > Since CUDA 10.2, there's some "[[ 
> > > > https://docs.nvidia.com/deploy/cuda-compatibility/ | compatibility mode 
> > > > ]]" that allows to run newer CUDA on older drivers. As long as you are 
> > > > not using the latest features, of course.
> > > > 
> > > > I'm personally all up for removing redundancy and duplication. 
> > > I'm fo

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca updated this revision to Diff 389678.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -127,7 +127,8 @@
   SmallVector Candidates;
 
   // In decreasing order so we prefer newer versions to older versions.
-  std::initializer_list Versions = {"8.0", "7.5", "7.0"};
+  std::initializer_list Versions = {
+  "11.5", "11.4", "11.3", "11.2", "11.1", "11.0", "10.2", "10.1", "10.0"};
   auto &FS = D.getVFS();
 
   if (Args.hasArg(clang::driver::options::OPT_cuda_path_EQ)) {


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -127,7 +127,8 @@
   SmallVector Candidates;
 
   // In decreasing order so we prefer newer versions to older versions.
-  std::initializer_list Versions = {"8.0", "7.5", "7.0"};
+  std::initializer_list Versions = {
+  "11.5", "11.4", "11.3", "11.2", "11.1", "11.0", "10.2", "10.1", "10.0"};
   auto &FS = D.getVFS();
 
   if (Args.hasArg(clang::driver::options::OPT_cuda_path_EQ)) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Mojca Miklavec via Phabricator via cfe-commits]

mojca created this revision.
mojca added reviewers: tra, carlosgalvezp, Hahnfeld.
mojca added a project: clang.
Herald added a subscriber: yaxunl.
mojca requested review of this revision.
Herald added a subscriber: cfe-commits.

This is heavily related to https://reviews.llvm.org/D114326 and uses a 
different approach for locating path to CUDA on Windows.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114601

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -135,10 +135,7 @@
 Candidates.emplace_back(
 Args.getLastArgValue(clang::driver::options::OPT_cuda_path_EQ).str());
   } else if (HostTriple.isOSWindows()) {
-for (const char *Ver : Versions)
-  Candidates.emplace_back(
-  D.SysRoot + "/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v" +
-  Ver);
+Candidates.emplace_back(llvm::sys::Process::GetEnv("CUDA_PATH"));
   } else {
 if (!Args.hasArg(clang::driver::options::OPT_cuda_path_ignore_env)) {
   // Try to find ptxas binary. If the executable is located in a directory


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -135,10 +135,7 @@
 Candidates.emplace_back(
 Args.getLastArgValue(clang::driver::options::OPT_cuda_path_EQ).str());
   } else if (HostTriple.isOSWindows()) {
-for (const char *Ver : Versions)
-  Candidates.emplace_back(
-  D.SysRoot + "/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v" +
-  Ver);
+Candidates.emplace_back(llvm::sys::Process::GetEnv("CUDA_PATH"));
   } else {
 if (!Args.hasArg(clang::driver::options::OPT_cuda_path_ignore_env)) {
   // Try to find ptxas binary. If the executable is located in a directory
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

I opened https://reviews.llvm.org/D114601. I wasn't sure if that's something 
that should have been combined with this ticket or not because it can be merged 
or rejected independently.

I also opened a ticket for Microsoft STL on 
https://github.com/microsoft/STL/issues/2359.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:138
   } else if (HostTriple.isOSWindows()) {
-for (const char *Ver : Versions)
-  Candidates.emplace_back(
-  D.SysRoot + "/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v" +
-  Ver);
+Candidates.emplace_back(llvm::sys::Process::GetEnv("CUDA_PATH"));
   } else {

Sorry, wrong patch. I'll upload it again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Mojca Miklavec via Phabricator via cfe-commits]

mojca updated this revision to Diff 389808.

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

https://reviews.llvm.org/D114601

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -135,10 +135,7 @@
 Candidates.emplace_back(
 Args.getLastArgValue(clang::driver::options::OPT_cuda_path_EQ).str());
   } else if (HostTriple.isOSWindows()) {
-for (const char *Ver : Versions)
-  Candidates.emplace_back(
-  D.SysRoot + "/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v" +
-  Ver);
+Candidates.emplace_back(llvm::sys::Process::GetEnv("CUDA_PATH"));
   } else {
 if (!Args.hasArg(clang::driver::options::OPT_cuda_path_ignore_env)) {
   // Try to find ptxas binary. If the executable is located in a directory


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -135,10 +135,7 @@
 Candidates.emplace_back(
 Args.getLastArgValue(clang::driver::options::OPT_cuda_path_EQ).str());
   } else if (HostTriple.isOSWindows()) {
-for (const char *Ver : Versions)
-  Candidates.emplace_back(
-  D.SysRoot + "/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v" +
-  Ver);
+Candidates.emplace_back(llvm::sys::Process::GetEnv("CUDA_PATH"));
   } else {
 if (!Args.hasArg(clang::driver::options::OPT_cuda_path_ignore_env)) {
   // Try to find ptxas binary. If the executable is located in a directory
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Mojca Miklavec via Phabricator via cfe-commits]

mojca updated this revision to Diff 389811.

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

https://reviews.llvm.org/D114601

Files:
  clang/lib/Driver/ToolChains/Cuda.cpp


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -134,10 +134,11 @@
 Candidates.emplace_back(
 Args.getLastArgValue(clang::driver::options::OPT_cuda_path_EQ).str());
   } else if (HostTriple.isOSWindows()) {
-for (const char *Ver : Versions)
-  Candidates.emplace_back(
-  D.SysRoot + "/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v" +
-  Ver);
+if (Optional CudaPathValue =
+llvm::sys::Process::GetEnv("CUDA_PATH")) {
+  StringRef CudaPath = *CudaPathValue;
+  Candidates.emplace_back(CudaPath.str());
+}
   } else {
 if (!Args.hasArg(clang::driver::options::OPT_cuda_path_ignore_env)) {
   // Try to find ptxas binary. If the executable is located in a directory


Index: clang/lib/Driver/ToolChains/Cuda.cpp
===
--- clang/lib/Driver/ToolChains/Cuda.cpp
+++ clang/lib/Driver/ToolChains/Cuda.cpp
@@ -134,10 +134,11 @@
 Candidates.emplace_back(
 Args.getLastArgValue(clang::driver::options::OPT_cuda_path_EQ).str());
   } else if (HostTriple.isOSWindows()) {
-for (const char *Ver : Versions)
-  Candidates.emplace_back(
-  D.SysRoot + "/Program Files/NVIDIA GPU Computing Toolkit/CUDA/v" +
-  Ver);
+if (Optional CudaPathValue =
+llvm::sys::Process::GetEnv("CUDA_PATH")) {
+  StringRef CudaPath = *CudaPathValue;
+  Candidates.emplace_back(CudaPath.str());
+}
   } else {
 if (!Args.hasArg(clang::driver::options::OPT_cuda_path_ignore_env)) {
   // Try to find ptxas binary. If the executable is located in a directory
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

Somewhat off-topic from a discussion earlier in the thread.
What's the purpose of the following code then if users are supposed to 
explicitly specify the `-L` flag anyway?

  c++
  if (HostTriple.isArch64Bit() && FS.exists(InstallPath + "/lib64"))
LibPath = InstallPath + "/lib64";
  else if (FS.exists(InstallPath + "/lib"))
LibPath = InstallPath + "/lib";
  else
continue;

The code above may be improved for Windows (it needs a different path there), 
for example like this:

  c++
  if (HostTriple.isOSWindows() && (HostTriple.getArch() == 
llvm::Triple::ArchType::x86_64) && FS.exists(InstallPath + "/lib/x64"))
LibPath = InstallPath + "/lib/x64";
  if (HostTriple.isArch64Bit() && FS.exists(InstallPath + "/lib64"))
LibPath = InstallPath + "/lib64";
  else if (FS.exists(InstallPath + "/lib"))
LibPath = InstallPath + "/lib";
  else
continue;

but I fail to figure out how to make use of this variable.

CMake might be able to add the right flags automatically (but it doesn't 
currently support Clang with CUDA on Windows), but writing the following down 
"manually" is relatively annoying:

  clang++.exe hello.cu -o hello --cuda-path="C:/Program Files/NVIDIA GPU 
Computing Toolkit/CUDA/v11.4" -l cudart -L "C:/Program Files/NVIDIA GPU 
Computing Toolkit/CUDA/v11.4/lib/x64"

Also, it's probably destined to lead into issues if the user needs to manually 
specify the library path, while the cuda path is determined automatically.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

@tra: what should be done about the unit test that checks whether CUDA SDK 8.0 
specifically has been found?
Apparently you are taking care of the buildbot configuration performing those 
tests.

And how should we proceed in general, what can I do next? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

@tra: this is not yet 100% ready since the unit tests are now failing 
(expecting to find CUDA 8.0).
I can fix the unit test, but I suppose that someone needs to install additional 
SDK somewhere into the infrastructure as well?

In D114326#3158913 , @tra wrote:

> So, yes, you could automatically add linking flags in your example, but it's 
> not feasible to do so for the linking phase in general. It would also 
> introduce inconsistency in clang's linking phase behavior and would lead to 
> questions -- why things work with `clang hello.co`, but not with `clang -c 
> hello.cu; clang hello.o` ?

Please note that I'm a complete newbie to compilers.
Te flag `--cuda-path=` slightly reminds me of the `--framework` keyword on 
macOS. I'm not behind a mac right now, so please forgive me any typo or wrong 
paths (it's also not exact since the frameworks are searched for in different 
folders etc.), below is just a general idea.

  # this one effectively adds 
-I/System/Library/Frameworks/OpenGL.framework/Headers
  clang++ -c hello.cpp --framework OpenGL
  
  # this one effectively adds -L/System/Library/Frameworks/OpenGL.framework/lib 
as well as the library itself (-l)
  clang++ hello.o --framework OpenGL
  
  # this one adds both include (-I) and linker directory (-L) flags
  clang++ hello.cpp --framework OpenGL

What would be cool to me was if `--cuda-path` was also implicitly adding the 
`-L` flag, so that the following would work:

  clang++ --cuda-path=/usr/local/cuda-11.5 -l cudart hello.cu
  clang++ --cuda-path=/usr/local/cuda-11.5 -l cudart hello.o

I don't know whether it would be acceptable to add (the default) cuda library 
path to the linker though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added inline comments.



Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:137
   } else if (HostTriple.isOSWindows()) {
-for (const char *Ver : Versions)
-  Candidates.emplace_back(

tra wrote:
> Do we want to keep this as the fall-back for cases when `CUDA_PATH` is not 
> set?
> 
> Otherwise, we're risking to break existing users.
That was going to be my question as well (for which I wanted your input).
From the initial comments in the other review (D114326) I understood that 
eventually you wanted to get rid of the hardcoded list of versions (but I 
didn't fully understand the intent).

(Technically speaking this won't break existing users since they were only able 
to access CUDA versions up to 8.0 until now, and we would just remove support 
for anything older that CUDA 10.0.)


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

https://reviews.llvm.org/D114601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

What can/should I do next in order to proceed with this?


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

https://reviews.llvm.org/D114601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

In D114601#3223155 , @tra wrote:

> Ping. @mojca, do you need help landing the patch?

Yes, please. I don't have commit access yet.
You can attribute it to mojca at macports.org, for example.

We also need a fix for unit tests on the CI in order to find the files, but I 
cannot help with that as I don't know how that works.


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

https://reviews.llvm.org/D114601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114601: Read path to CUDA from env. variable CUDA_PATH on Windows

[Mojca Miklavec via Phabricator via cfe-commits]

mojca added a comment.

Also, I would like to get to do some further "baby steps" towards better 
support of CUDA on Windows in particular, but I would need some guidelines.
I requested a special channel that would allow a bit of discussion 
https://discord.com/channels/636084430946959380/.
What would be the best way to shortly discuss the options?


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

https://reviews.llvm.org/D114601

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits