[Openmp-commits] [PATCH] D140783: [OpenMP][AMDGPU] Extract code object version from the ELF

[Saiyedul Islam via Phabricator via Openmp-commits]

saiislam added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:128
   std::queue<int> FreeKernargSegments;
+  uint16_t CodeObjectVersion;
 
----------------
JonChesterfield wrote:
> I don't think the memory pool needs to know what format the elf is using
Fixed in the new diff.

`kernargSizeIncludingImplicit()` needs the CodeObjectVersion because size of ImplicitArgs in cov4 is 56 bytes whereas it is 256 bytes in cov5. 


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:481
   std::vector<std::string> TargetID;
+  uint16_t CodeObjectVersion;
 
----------------
JonChesterfield wrote:
> This is per code object, how does a global scalar capture that?
As we do not support mixing of code object versions, the value remains the same for all.

This field gets set during call to `__tgt_rtl_load_binary_locked()` and is later used as `DeviceInfo().CodeObjectVersion`.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1382
+
+  if (const auto *ELFObj = dyn_cast<ELF64LEObjectFile>(ElfOrErr->get())) {
+    auto Header = ELFObj->getELFFile().getHeader();
----------------
JonChesterfield wrote:
> This seems likely to be something already available in the elf library 
I took the style that two other functions are using for looking up symbols and getting platform flags in this file. *.getHeader() seem to be the simplest way to get the header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140783