[Openmp-commits] [PATCH] D124525: [OpenMP][ClangLinkerWrapper] Extending linker wrapper to embed metadata for multi-arch fat binaries

[Saiyedul Islam via Phabricator via Openmp-commits]

saiislam added a comment.

Thanks for the detailed review. I will update rest of the patch soon.



================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:613-614
   CmdArgs.push_back("-shared");
+  std::string ArchArg = std::string("-plugin-opt=mcpu=").append(Arch.str());
+  CmdArgs.push_back(ArchArg);
   CmdArgs.push_back("-o");
----------------
jhuber6 wrote:
> Does this just pass the architecture to the AMD link? Is this related? If not move it to a separate patch and I'll review it.
Yes, it passes the architecture to AMD link.


================
Comment at: clang/tools/clang-linker-wrapper/OffloadWrapper.cpp:229-232
+    // store value of these variables (i.e. offload archs) into a custom
+    // section which will be used by "offload-arch -f". It won't be
+    // removed during binary stripping.
+    GV->setSection(".offload_arch_list");
----------------
jhuber6 wrote:
> Why does this need a custom section? We should just use it like this, not sure why we need these to  be anything but some internal struct.
> ```
> for (auto *Image : BinDesc->Images) {
>   if (Image->Info)
>    // Use Info
> }
> ```
Custom section is required so that it survives binary stripping done by various OSes. `Offload-arch` tool will look into this section of the binary to print list of supported architectures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124525