[Openmp-commits] [PATCH] D137607: Add interop support for OpenMP AMD GPU plugin

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

Where are the tests?



================
Comment at: openmp/libomptarget/include/omptargetplugin.h:166
+// Retrieve backend driver information
+void __tgt_rtl_get_driver_info(int32_t *DriverInfo);
+
----------------
Why don't we "just" return the int32_t? The indirection seems counterintuitive to me. Also, "DriverInfo" is not really descriptive. Can you specify what it returns, wrt. standard defined structs etc.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2689
+
+  if ((int)(DeviceInfo().HSAQueueSchedulers.size()) < DeviceId) {
+    // If the cooresponding device is not available, then set the error code to
----------------
Please don't cast to "int". If anything, pick a explicit size.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2698
+      return OFFLOAD_FAIL;
+    } else {
+      (*AsyncInfo)->Queue = Queue;
----------------
jdoerfert wrote:
> No return after else.
No else after return (is what I meant). Also other places.


================
Comment at: openmp/libomptarget/src/interop.cpp:91
   case omp_ipr_vendor_name:
+  case omp_ipr_fr_name:
     return getVendorIdToStr(InteropVal.vendor_id);
----------------
This seems odd. Is the runtime and the vendor the same thing for us?


================
Comment at: openmp/libomptarget/src/interop.cpp:217
+      !Device.RTL->get_driver_info) {
+    delete InteropPtr;
+    InteropPtr = omp_interop_none;
----------------
Do you know it's null or allocated here?


================
Comment at: openmp/libomptarget/src/interop.cpp:232
+                                       omp_interop_backend_type_cuda_1);
+  }
+  // InteropPtr = new omp_interop_val_t(DeviceId, InteropType);
----------------
Why do we need this new function if we call init device info afterwards anyway? It seems we now have two functions we always call in sequence to get two numbers out of the plugin. If so, we should merge them.


================
Comment at: openmp/libomptarget/src/interop.cpp:233
+  }
+  // InteropPtr = new omp_interop_val_t(DeviceId, InteropType);
+  if (Device.RTL->init_device_info(DeviceId, &(InteropPtr)->device_info,
----------------
Leftover?


================
Comment at: openmp/libomptarget/src/interop.cpp:245
+    return;
   }
+
----------------
Why is the ready check happening after the rest, could you explain the problem here?


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

https://reviews.llvm.org/D137607