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

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added inline comments.


================
Comment at: openmp/libomptarget/src/rtl.cpp:104
+}
+
 void RTLsTy::loadRTLs() {
----------------
JonChesterfield wrote:
> 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?
> 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.
I actually was basing this off of HSA's reference counting, which looks close to the current implementation.

> 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)
It contains a lot of mutexes, so I would guess not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131089