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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

This is still too complex and inconsistent:

1. Remove the old plugin API.
2. Always allocate an interop object in one place, not in 3, default initialize it.
3. If something goes wrong, set an error message and return the object.
4. Pass the interop to the plugin to be initialized.



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2692
+    // Queue field
+    (*AsyncInfo)->Queue = nullptr;
+
----------------
Here you write *AsyncInfo but only below you allocate memory for it. Either there is an object and you overwrite it below or there is not and this will segfault.


================
Comment at: openmp/libomptarget/src/interop.cpp:209
+    return;
+  }
+
----------------
No error message here?


================
Comment at: openmp/libomptarget/src/interop.cpp:211
-      Device.RTL->init_device_info(DeviceId, &(InteropPtr)->device_info,
-                                   &(InteropPtr)->err_str)) {
-    delete InteropPtr;
----------------
This API is now unused from the outside, right? Can we get rid of it?


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


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

https://reviews.llvm.org/D137607