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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

> The "tgt_rtl_init_device_info" and "tgt_rtl_init_async_info" were from CUDA plugin, so I'm not sure if we should remove "__tgt_rtl_init_device_info" since it was not used ourside.

I'm now very confused what public APIs exist and which ones are actually needed/used. We should have a single one, but I'm fairly sure that's not the case, right? Why not. I don't understand the sentence I quoted. Maybe elaborate a little more if you think we should have more than one external plugin API.



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2696
+    return OFFLOAD_FAIL;
+  }
+
----------------
There is an assert followed by a check, that seems wrong. Either it's a pre-condition or it's something we want to handle gracefully.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2696
+    return OFFLOAD_FAIL;
+  }
+
----------------
jdoerfert wrote:
> There is an assert followed by a check, that seems wrong. Either it's a pre-condition or it's something we want to handle gracefully.
A nullptr queue is not an error code. It is a perfectly valid state meaning there is no outstanding async work. Check the CUDA impl, it will just return OFFLOAD_FAIL if the DeviceId is invalid, do the same.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2720-2726
+  if (DeviceInfo().HSAAgents.size() <= DeviceId) {
+    // If the cooresponding device is not available, then set device as null
+    DeviceInfoPtr->Device = nullptr;
+    *ErrStr = "Device ID is invalid";
+
+    return OFFLOAD_FAIL;
+  }
----------------
There is an assert followed by a check, that seems wrong. Either it's a pre-condition or it's something we want to handle gracefully.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2729-2731
+  hsa_agent_t &Agent = DeviceInfo().HSAAgents[DeviceId];
+  if (!DeviceInfoPtr->Device) {
+    DeviceInfoPtr->Device = &Agent;
----------------



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:2742
+  InteropPtr->vendor_id = hip;
+  InteropPtr->backend_type_id = omp_interop_backend_type_hip_1;
+
----------------
We are actually not using `hip` at all. There is no backend or vendor part that is `hip`. We use `HSA`. Add a new enum value for both.


================
Comment at: openmp/libomptarget/src/interop.cpp:69
+  return ("unknown backend");
+}
+
----------------
Why the `(` `)`?


================
Comment at: openmp/libomptarget/test/api/omp_interop_amdgpu.c:77
+  return 0;
+}
----------------
Clang format this file please.


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

https://reviews.llvm.org/D137607