[Openmp-commits] [PATCH] D140973: [OpenMP][AMDGPU] Support of cov5 in the next gen plugin

Wed Jan 4 07:53:55 PST 2023

jdoerfert added a comment.

Thanks for updating the new plugin too. Some comments below.

================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:1666
+    new (AMDKernel)
+        AMDGPUKernelTy(KernelEntry.name, ExecModeGlobal.getValue(), Cov);

----------------
Is this a per-kernel property or a per image property? If it's the latter, maybe add a TODO to move it to a image abstraction once we have a AMD specific one.

================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:2134
 /// handler.
 struct AMDGPUGlobalHandlerTy final : public GenericGlobalHandlerTy {
   /// Get the metadata of a global from the device. The name and size of the
----------------
COV is AMD-GPU specific I assume, right? If so, put the implementation in here and a default impl into the generic part. Also, add documentation to each new method, please.

================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h:29
+  return Version < ELF::ELFABIVERSION_AMDGPU_HSA_V5 ? 56 : 256;
+}

----------------
I think we want to keep the layout, eventually we'll need it, right?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140973