[PATCH] D117246: [OpenMP] Add support for linking AMDGPU images

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:297
+
+    std::string Arch = DAL->getLastArgValue(options::OPT_march_EQ).str();
+    if (Arch.empty()) {
----------------
JonChesterfield wrote:
> This part is valuable as-is and probably independently testable, in that I think -### will have no march= before this change and will contain march=gfx*** after it. Though for that test to work reliably, I think it would have to run on a machine where amdgpu-arch succeeded, which is probably more bother than it's worth. Will check locally.
Yes, I believe with this we could remove the other calls to `checkSystemForAMDGPU` and just reference the `-march` in the driver arguments. Not sure about testing because it requires the binary of course.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:623
+  // Copy system library paths used by the host linker.
+  for (StringRef Arg : LinkerArgs)
+    if (Arg.startswith("-L"))
----------------
JonChesterfield wrote:
> Does this mean to take the library paths the host used to link, and pass them unchanged for the device link as well? Doesn't seem a given that this is the right behaviour. Do we have a mechanism for passing arguments to the device linker? Might be ugly, e.g. `-Xopenmp-target=sm_70 -Xopenmp-target=-Wl,whatever`
That's a good idea for general control over the arguments to the linker wrapper. I don't think the `-L` arguments are strictly necessary here. Could probably move to your approach if needed, the `-L` arguments were already present in the `clang-nvlink-wrapper` so I just replicated that without really caring if it was necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117246