[PATCH] D140158: [CUDA] Allow targeting NVPTX directly without a host toolchain

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.

LGTM with few minor nits and questions.

In D140158#4063689 <https://reviews.llvm.org/D140158#4063689>, @jhuber6 wrote:

> Addressing some comments. I don't know if there's a cleaner way to mess around with the `.cubin` nonsense. I liked symbolic links but that doesn't work on Windows.

Can we do a copy or rename instead. That could be more portable.



================
Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:518-521
+                                    const InputInfo &Output,
+                                    const InputInfoList &Inputs,
+                                    const ArgList &Args,
+                                    const char *LinkingOutput) const {
----------------
Nit: Looks like there are tabs.


================
Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:448-450
+  // If we are invoking `nvlink` internally we need to output a `.cubin` file.
+  // Checking if the output is a temporary is the cleanest way to determine
+  // this. Putting this logic in `getInputFIlename` isn't an option because it
----------------
jhuber6 wrote:
> tra wrote:
> > Can't say that I like this approach. It heavily relies on "happens to work".
> > 
> > Perhaps a better way to deal with this is to create the temporary GPU object file name with ".cubin" extension to start with.
> > 
> > 
> > 
> As far as I know, the files are created independently of the tool-chains so I'm not sure if we'd be able to check there. The current way is to use `getInputFilename` but that doesn't have access to the compilation. As far as I can come up with there's the following solutions
> 
> - Do this and check the temp files
> - Create a symbolic link if the file is empty (Doesn't work on Windows)
> - Make a random global that's true if the Linker tool was built at some point
Naming is hard. :-(

OK, let's add a FIXME to this comment and hope to get it fixed when NVIDIA's tools become less obsessive about file extensions. I'll file a bug with them to get the ball rolling on their end.


================
Comment at: clang/lib/Driver/ToolChains/Cuda.h:196-197
+
+   void AddCudaIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                            llvm::opt::ArgStringList &CC1Args) const override;
 
----------------
jhuber6 wrote:
> tra wrote:
> > Nit: it's hard to tell whether the whitespace additions are spaces or tabs. They show up as ">" to me which suggests it may be tabs. Just in case it is indeed the case, please make sure to un-tabify the changes.
> I think it's because the original file was formatted incorrectly, so all my new changes are formatted correctly. I'm somewhat more tempted to just clang-format it upstream and rebase it.
Clang-formatting the file(s) and submitting that change separately before the patch SGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140158