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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 marked 3 inline comments as done.
jhuber6 added a comment.

In D140158#4063720 <https://reviews.llvm.org/D140158#4063720>, @tra wrote:

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

I do a copy in this patch already. The problem is that if it's an internal link, e.g. `clang --target=nvptx64-nvidia-cuda foo.c bar.c`, then there will be no file to copy. The symbol link got around this by linking the files so once the `.o` was written the contents would automatically appear in the `.cubin`. The other approach is to detect this and use the correct names which is what this patch does, albeit a little jankily.



================
Comment at: clang/lib/Driver/ToolChains/Cuda.cpp:518-521
+                                    const InputInfo &Output,
+                                    const InputInfoList &Inputs,
+                                    const ArgList &Args,
+                                    const char *LinkingOutput) const {
----------------
tra wrote:
> Nit: Looks like there are tabs.
I think that's just the diff showing that it was indented, there's no tabs in the file.


================
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
----------------
tra wrote:
> 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.
I would like that. it's a very simple check if you actually know how ELF headers work... Also while you're at it, ask for how they set their machine flags in Nvidia so we can display the arch via `llvm-readelf`.

I'll add some FIXME's.


================
Comment at: clang/lib/Driver/ToolChains/Cuda.h:196-197
+
+   void AddCudaIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                            llvm::opt::ArgStringList &CC1Args) const override;
 
----------------
tra wrote:
> 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.
Already did :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140158