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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

Comments inline. I'd like to use the same hook to fix the memory manager lifetime error



================
Comment at: openmp/libomptarget/src/interface.cpp:95
   TIMESCOPE();
+  std::call_once(PM->RTLs.initFlag, &RTLsTy::LoadRTLs, PM->RTLs);
+  for (auto &RTL : PM->RTLs.AllRTLs)
----------------
I suspect we should call loadrtls from the libomptarget init but that could be a different patch


================
Comment at: openmp/libomptarget/src/interface.cpp:108
+  for (auto &RTL : PM->RTLs.UsedRTLs)
+    if (RTL->unregister_lib)
+      RTL->unregister_lib(desc);
----------------
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


================
Comment at: openmp/libomptarget/src/rtl.cpp:26
 static const char *RTLNames[] = {
+    /* Remote target        */ "libomptarget.rtl.rpc.so",
     /* PowerPC target       */ "libomptarget.rtl.ppc64.so",
----------------
Goes at the end, precedence given to first to ship


================
Comment at: openmp/libomptarget/src/rtl.cpp:271
 void RTLsTy::RegisterLib(__tgt_bin_desc *desc) {
-  // Attempt to load all plugins available in the system.
-  std::call_once(initFlag, &RTLsTy::LoadRTLs, this);
----------------
Moving this out of registerlib looks great


================
Comment at: openmp/libomptarget/src/rtl.h:56
   typedef int64_t(synchronize_ty)(int32_t, __tgt_async_info *);
+  typedef void(register_lib_ty)(__tgt_bin_desc *);
+  typedef void(register_requires_ty)(int64_t);
----------------
Functions should return non-void so they can indicate failure. Also, prefer function pointer to function type for clarity - you're missing an asterisk


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93293