[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:44
 
-static char *ProfileTraceFile = nullptr;
+static std::mutex PluginManagerMutex;
 
----------------
jhuber6 wrote:
> JonChesterfield wrote:
> > This probably puts a call in global ctors. Suggest the uglier pthread alternative which doesn't do that.
> Do you have an example of that? It wouldn't work with `std::scoped_lock` then right.
pthread's mutex has an initialiser form that emits no code and lazily sets up the mutex. Using it with scoped lock would require some adapter thing which is a pain. The C++ mutex is definitely prettier, I'm just conscious that it's also at the root of stack traces for HSA segfaulting during initialisation, and can see a potential analogy here. Not a big deal to ship the C++ one and then replace it if it blows up the same way.


================
Comment at: openmp/libomptarget/src/rtl.cpp:104
+}
+
 void RTLsTy::loadRTLs() {
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131089