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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added a comment.

Could you explain briefly what the approach here is? I'm confused as to what's actually changed and how we're handling this difference. I thought if this was just the definition of some builtin function we could just rely on the backend to figure it out. Why do we need to know the code object version inside the device RTL?



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:17140
+
+  if (Cov == clang::TargetOptions::COV_None) {
+    auto *ABIVersionC = CGF.CGM.GetOrCreateLLVMGlobal(
----------------
Could you explain the function of this in a comment? Are we emitting generic code if unspecified?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:17187-17188
+        Address(Result, CGF.Int16Ty, CharUnits::fromQuantity(2)));
+  } else {
+    if (Cov == clang::TargetOptions::COV_5) {
+      // Indexing the implicit kernarg segment.
----------------
nit.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:17194
+      // CGBuiltin.cpp ~ line 17052 ~ Value*EmitAMDGPUWorkGroupSize ~ COV:
+      // " << Cov ; llvm::errs().resetColor();
+    } else {
----------------
Leftover debugging?


================
Comment at: clang/lib/Driver/ToolChain.cpp:1360
+    if (A->getOption().matches(options::OPT_mcode_object_version_EQ))
+      DAL->append(A);
+
----------------
Shouldn't we be able to put this under the `OPT_m_group` below?


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1752
+    // if (auto Err = preAllocateDeviceMemoryPool())
+    //   return Err;
+
----------------
Leftoever?


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:2542
+  /// Get the address of pointer to the preallocated device memory pool.
+  void **getPreAllocatedDeviceMemoryPool() {
+    return &PreAllocatedDeviceMemoryPool;
----------------
Why do we need this? The current method shouldn't need to change if all we're doing is allocating memory of greater size.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:3036
 
+  if (getImplicitArgsSize() < utils::COV5_SIZE) {
+    DP("Setting fields of ImplicitArgs for COV4\n");
----------------
So we're required to emit some new arguments? I don't have any idea what'schanged between this COV4 and COV5 stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139730