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

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added a comment.

I'm not fully up-to-date, what's the main difference and advantage of the new code object version? What do all the new implicit arguments do.



================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:953
       getAMDGPUCodeObjectVersion(getDriver(), DriverArgs));
+
   if (!RocmInstallation.checkCommonBitcodeLibs(CanonArch, LibDeviceFile,
----------------
Unrelated?


================
Comment at: openmp/libomptarget/DeviceRTL/include/Interface.h:169
+
+#ifdef __AMDGCN__
+size_t external_get_local_size(uint32_t dim);
----------------
This should probably use variants to match the rest of the style, also if you intend to read these outside of the library you'll need to put them in the exports file and set their visibility.


================
Comment at: openmp/libomptarget/DeviceRTL/src/Mapping.cpp:19
 #pragma omp begin declare target device_type(nohost)
-
+extern const uint16_t __oclc_ABI_version;
 #include "llvm/Frontend/OpenMP/OMPGridValues.h"
----------------
What if this isn't defined? We should be able to use the OpenMP library without the AMD device libraries. Should it be extern weak?


================
Comment at: openmp/libomptarget/DeviceRTL/src/State.cpp:73
+extern "C" {
+#ifdef __AMDGCN__
+size_t external_get_local_size(uint32_t dim) {
----------------
Variants


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/get_elf_mach_gfx_name.h:15
 
+enum IMPLICITARGS : uint16_t {
+  COV4_SIZE = 56,
----------------
Unrelated, but is there any particular reason these aren't defined in the `hsa_amd_ext.h`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139730