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

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added a comment.

LG overall, some final nits.



================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:444-445
+    if (auto Err = KernelInfoOrErr.takeError()) {
+      INFO(OMP_INFOTYPE_PLUGIN_KERNEL, Device.getDeviceId(),
+           "Could not read extra information for kernel %s", getName());
+      KernelInfo = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
----------------
Since we're ignoring this, it would be best to use `std::optional`. But just for reference, you would need to use `consumeError` here to actually ignore it. You could go ahead and make the `KernelInfo` variable an optional as well, so we don't need to bother with a zero fill or anything.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h:237
+  Error generateKernelInfo(msgpack::ArrayDocNode::ArrayTy::iterator It) {
+    KernelMetaDataTy KernelData = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+    std::string KernelName;
----------------
Default constructor should be fine here.


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h:162
+    auto Name = Note.getName();
+    auto Type = Note.getType();
+    if (Name != "AMDGPU")
----------------
Nit, but these probably don't need to be their own variables since the getters are self-explanatory and used only once.


================
Comment at: openmp/libomptarget/test/offloading/info.c:1
-// RUN: %libomptarget-compile-generic \
 // RUN:     -gline-tables-only -fopenmp-extensions
----------------
This should stay generic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144521