[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a reviewer: rnk.
Hahnfeld added a subscriber: rnk.
Hahnfeld added a comment.

Please format all comments as full sentences.



================
Comment at: lib/Driver/ToolChains/Cuda.cpp:338
+        Args.getAllArgValues(options::OPT_march_EQ);
+    assert(GPUArchs.size() == 1 && "Exactly one GPU Arch required for ptxas.");
+    const std::string &GPUArch = GPUArchs[0];
----------------
Is this catched somewhere before with a diagnostic?


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:381
+      StringRef Name = llvm::sys::path::filename(II.getFilename());
+      std::pair<StringRef, StringRef> Split = Name.rsplit('.');
+      std::string TmpName =
----------------
`AddOpenMPLinkerScript` uses `llvm::sys::path::replace_extension` which does this magic internally


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:388-394
+      const char *CopyExec = Args.MakeArgString(getToolChain().GetProgramPath(
+          C.getDriver().IsCLMode() ? "copy" : "cp"));
+      ArgStringList CopyCmdArgs;
+      CopyCmdArgs.push_back(II.getFilename());
+      CopyCmdArgs.push_back(CubinF);
+      C.addCommand(
+          llvm::make_unique<Command>(JA, *this, CopyExec, CopyCmdArgs, Inputs));
----------------
Copying files around would be something new in Clang that someone more experienced has to weight. @rnk, @hfinkel?


Repository:
  rL LLVM

https://reviews.llvm.org/D29654