[PATCH] D42800: Let CUDA toolchain support amdgpu target

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

Only commenting on parts that I'm a bit familiar with. In general, does it make sense to split this patch, are there different "stages" of support? Like 1) being able to compile an empty file, 2) generate optimized code, 3) allow using math functions?



================
Comment at: lib/Driver/ToolChains/Cuda.cpp:403-415
+  // If Verbose, save input for llc in /tmp and print all symbols
+  if (Args.hasArg(options::OPT_v)) {
+    ArgStringList nmArgs;
+    nmArgs.push_back(ResultingBitcodeF);
+    nmArgs.push_back("-debug-syms");
+    const char *nmExec = Args.MakeArgString(C.getDriver().Dir + "/llvm-nm");
+    C.addCommand(llvm::make_unique<Command>(JA, *this, nmExec, nmArgs, Inputs));
----------------
This never gets cleaned up!


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:531-534
+  SmallString<256> OutputFileName(Output.getFilename());
+  if (JA.isOffloading(Action::OFK_OpenMP))
+    llvm::sys::path::replace_extension(OutputFileName, "cubin");
+  CmdArgs.push_back(Args.MakeArgString(OutputFileName));
----------------
That is already done in `TC.getInputFilename(Output)` (since rC318763), the same function call that you are removing here...


================
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(
----------------
`clang-fixup-fatbin` is not upstreamed and won't work. Sounds like a horrible name btw...


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:788-793
+  // Do not add -link-cuda-bitcode or ptx42 features if gfx
+  for (Arg *A : DriverArgs)
+    if (A->getOption().matches(options::OPT_cuda_gpu_arch_EQ) &&
+        StringRef(A->getValue()).startswith("gfx"))
+      return;
+
----------------
You should use `GpuArch` which comes from `DriverArgs.getLastArgValue`: The last `-march` overrides previous arguments.


https://reviews.llvm.org/D42800