[Openmp-commits] [PATCH] D131089: [Libomptarget] Explicitly init / deinit libomptarget from the user

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/src/rtl.cpp:104
+}
+
 void RTLsTy::loadRTLs() {
----------------
JonChesterfield wrote:
> jhuber6 wrote:
> > JonChesterfield wrote:
> > > I think the above is right but it took me a while to parse. Control flow and data flow are a bit interleaved. Could we go with:
> > > 
> > > ```
> > > if (PM == nullptr) {
> > >   PM == new PluginManager();
> > >   PM->Refcount = 1; // Perhaps the plugin manager should init it to 1 on construction?
> > >   PM->Load();
> > > }
> > > ```
> > > 
> > > and
> > > ```
> > > uint64_t before = PM->Refcount.fetch_sub(1);
> > > if (before == 1) {
> > >   PM->Unload();
> > >   delete PM;
> > > }
> > > ```
> > > 
> > > It seems somewhat strange that the plugin manager is a global object pointing to the heap. I wonder if that can be simplified, independent of this.
> > Logically I just wanted the `load / unload` routines to fire when the reference count is one.
> I think the code I suggested is isomorphic to yours, am I missing something?
> 
> Reference counting normally uses zero as the sentinel value as the zero to one transition can only occur when there's a single owner, though in this case the whole thing is wrapped in a mutex as well which makes that less important.
> 
> On which note, we could actually reference count this and delete the mutex if so inclined, but I think we'd need to statically allocate the plugin manager instead of heap allocate it. I wonder how complicated the state setup of that manager is (i.e. can it be trivially constructible)
Plugin manager is really complicated, probably don't want to statically allocate it. Maybe more readable to drop the atomic qualifier on the reference count, since it's always mutated under a lock?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131089