[PATCH] D42800: Let CUDA toolchain support amdgpu target

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

Let's start with the obvious points:

1. The latest patch clearly wasn't run through `clang-format`.
2. It only has some 90 lines of context which makes it look like you deleted quite some code when browsing through the history.
3. This patch is still large, did you at least consider splitting it as I proposed some weeks ago?

Additionally I don't see responses to all points that @tra raised. Their answers will probably explain the approach you chose, so I think they should also be added to the summary.



================
Comment at: lib/Basic/Cuda.cpp:290-302
+  case CudaArch::GFX701:
+  case CudaArch::GFX702:
+  case CudaArch::GFX703:
+  case CudaArch::GFX704:
+  case CudaArch::GFX800:
+  case CudaArch::GFX801:
+  case CudaArch::GFX802:
----------------
This means you can't compile when the Clang driver detected an installation of CUDA 9?


================
Comment at: lib/Basic/Targets/AMDGPU.h:383
   uint64_t getNullPointerValue(LangAS AS) const override {
+    if(getTriple().getOS()==llvm::Triple::CUDA) return 0;
     return AS == LangAS::opencl_local ? ~0 : 0;
----------------
How can `LangAS::opencl_local` ever be used in CUDA? I think this check is redundant.


================
Comment at: lib/Driver/Driver.cpp:3262-3263
            !C.getArgs().hasArg(options::OPT_traditional_cpp) && !SaveTemps &&
+           (InputType != types::TY_LLVM_IR) &&
+           (InputType != types::TY_LLVM_BC) &&
            !C.getArgs().hasArg(options::OPT_rewrite_objc);
----------------
I'm not sure I understand this change to the generic driver code: How can LLVM IR / BC ever need a preprocessor?


================
Comment at: lib/Driver/Driver.cpp:3954-3957
+    if (((StringRef)BaseInput).endswith(".a"))
+      Suffixed += "a";
+    else
+      Suffixed += Suffix;
----------------
If this is really needed this deserves some justification.


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:359-361
+  addBCLib(C, Args, CmdArgs, LibraryPaths, "opencl.amdgcn.bc");
+  addBCLib(C, Args, CmdArgs, LibraryPaths, "ockl.amdgcn.bc");
+  addBCLib(C, Args, CmdArgs, LibraryPaths, "irif.amdgcn.bc");
----------------
gregrodgers wrote:
> arsenm wrote:
> > Why is this done under an NVPTX:: class
> Because we are not creating a new toolchain for AMDGCN.  We modify the logic in the tool constructor as needed for AMDGCN, keeping the ability to provide a set of mixed targets. 
That sounds more like a hack, the CUDA classes should be separated from NVPTX and AMDGCN.


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:639-640
+    CmdArgs2.push_back(Args.MakeArgString(Output.getFilename()));
+    const char *Exec2 =
+        Args.MakeArgString(C.getDriver().Dir + "/clang-fixup-fatbin");
+    C.addCommand(
----------------
gregrodgers wrote:
> Hahnfeld wrote:
> > `clang-fixup-fatbin` is not upstreamed and won't work. Sounds like a horrible name btw...
> Major cleanup here in the next update.  It is not a bad name when you see the update and the comments in the update. 
I wasn't only criticizing the name but also the fact that this code won't work with only upstream components!


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:293
+  if (useOpenHeaders()) {
+    CC1Args.push_back("__clang_cuda_runtime_wrapper_open.h");
+    CC1Args.push_back("-D__USE_OPEN_HEADERS__");
----------------
I don't see this file in the upstream repository?


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:413
+
+  addEnvListWithSpaces(Args, CmdArgs, "CLANG_TARGET_LINK_OPTS");
+  CmdArgs.push_back("-suppress-warnings");
----------------
I don't think you should invent new environment variables, Clang normally uses the `-X` class to pass arguments to specific parts of the toolchain.


https://reviews.llvm.org/D42800