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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
