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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/src/interface.cpp:108
+  for (auto &RTL : PM->RTLs.UsedRTLs)
+    if (RTL->unregister_lib)
+      RTL->unregister_lib(desc);
----------------
atmnpatel wrote:
> 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.
Pattern is usually:
- generic construction
- specific construction
- use the objects
- specific destruction
- generic destruction

Lines up with base/derived patterns and lexical scoping of lifetimes.

If swapping generic + specific makes more sense here (which it might) then cool. The order of destruction should definitely be the opposite of the order of construction though. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93293