[Openmp-commits] [PATCH] D144521: [OpenMP][AMDGPU] More detail in AMDGPU kernel launch info

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added a comment.

Some initial nits. I wonder if we could move this code somewhere common, since both `llvm-readobj` and here wants to use it now. I don't know much about the actual msgpack format, maybe @JonChesterfield can chime in.



================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:2220-2222
+                                                    KernelInfoMap)) {
+    return Err;
+  }
----------------
nit: remove braces.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:2615-2616
+  // Only do all this when the output is requested
+  if (getInfoLevel() & OMP_INFOTYPE_PLUGIN_KERNEL) {
+    // General Info
+    auto DeviceId = GenericDevice.getDeviceId();
----------------
nit. prefer early exit.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h:153-154
+
+using ELFT = llvm::object::ELFType<llvm::support::little, true>;
+using ELF_Note = ELFT::Note;
+
----------------
Probably not needed. We only support the existing `llvm::object::ELF64LE` alias, which is exactly what you have here. The Note alias is only used a single time so it's probably fineto just use `ELF64LE::Note`.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h:168-170
+    if (Name != "AMDGPU") {
+      return Error::success(); // We are not interested in other things
+    }
----------------
nit. no braces around single line block here and below


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h:272
+    for (auto It = KernelsArr.begin(); It != KernelsArr.end(); ++It) {
+      if (!(*It).isMap()) {
+        MESSAGE0("Array entry not a map");
----------------
jhuber6 wrote:
> I'd guess that this should be an error. If we fail to parse the code object something is probably wrong.



================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h:272-274
+      if (!(*It).isMap()) {
+        MESSAGE0("Array entry not a map");
+      }
----------------
I'd guess that this should be an error. If we fail to parse the code object something is probably wrong.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:246-249
+  Error Err = printLaunchInfo(GenericDevice, KernelArgs, NumThreads, NumBlocks);
+  if (Err) {
+    return Err;
+  }
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144521