[PATCH] D139730: [OpenMP][DeviceRTL][AMDGPU] Support code object version 5

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

Codegen parts LGTM, questions with the driver parts



================
Comment at: clang/lib/Driver/ToolChain.cpp:1368
     if (A->getOption().matches(options::OPT_m_Group)) {
-      if (SameTripleAsHost)
+      // Pass code objection version to device toolchain
+      // to correctly set meta-data in intermediate files.
----------------
Typos


================
Comment at: clang/lib/Driver/ToolChain.cpp:1369
+      // Pass code objection version to device toolchain
+      // to correctly set meta-data in intermediate files.
+      if (SameTripleAsHost ||
----------------



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8649-8650
 
+  // code-object-version=X needs to be passed to clang-linker-wrapper to ensure
+  // that it is used by lld.
+  if (const Arg *A = Args.getLastArg(options::OPT_mcode_object_version_EQ)) {
----------------
so device rtl is linked once as a normal library?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8653-8654
+    CmdArgs.push_back(Args.MakeArgString("-mllvm"));
+    CmdArgs.push_back(Args.MakeArgString(
+        Twine("--amdhsa-code-object-version=") + A->getValue()));
+  }
----------------
Why do you need this? The code object version is supposed to come from a module flag. We should be getting rid of the command line argument for it


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:406-410
+  // pass on -mllvm options to the clang
+  for (const opt::Arg *Arg : Args.filtered(OPT_mllvm)) {
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Arg->getValue());
+  }
----------------
Shouldn't need this?


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:417
     CmdArgs.push_back("-save-temps");
+    // CmdArgs.push_back(Args.MakeArgString("--amdhsa-code-object-version=5"));
+  }
----------------
Commented out code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139730