[Openmp-commits] [PATCH] D93293: [OpenMP][Libomptarget] Introduce changes to support remote plugin

[Atmn Patel via Phabricator via Openmp-commits]

atmnpatel added inline comments.


================
Comment at: openmp/libomptarget/src/interface.cpp:118
+      if ((*RTL->unregister_lib)(desc) != OFFLOAD_SUCCESS) {
+        DP("Could not register library with %s", RTL->RTLName.c_str());
+      }
----------------
I'm assuming we can get away with just outputting a debug message for when the calls fail? I don't think its right to abort...


================
Comment at: openmp/libomptarget/src/interface.cpp:108
+  for (auto &RTL : PM->RTLs.UsedRTLs)
+    if (RTL->unregister_lib)
+      RTL->unregister_lib(desc);
----------------
JonChesterfield wrote:
> Not sure whether the new calls should be before or after the existing one, but the order should be reversed for unregister relative to register to nest the lifetimes
I kept it in this order because the remote plugin implementation receives __tgt_bin_desc(s) and uses that call to transfer the images/offload entries exactly once, and since RegisterLib asks if images are valid against RTLs, I needed to have libomptarget send the device images first, but if there's a compelling reason to swap the order, I'm happy to swap it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93293