[Libomptarget] Remove global ctor and use reference counting #80499

jhuber6 · 2024-02-02T22:13:07Z

Summary:
Currently we rely on global constructors to initialize and shut down the
OpenMP runtime library and plugin manager. This causes some issues
because we do not have a defined lifetime that we can rely on to release
and allocate resources. This patch instead adds some simple reference
counted initialization and deinitialization function.

A future patch will use the deinit interface to more intelligently
handle plugin deinitilization. Right now we do nothing and rely on
atexit inside of the plugins to tear them down. This isn't great
because it limits our ability to control these things.

Note that I made the __tgt_register_lib functions do the
initialization instead of adding calls to the new runtime functions in
the linker wrapper. The reason for this is because in the past it's been
easier to not introduce a new function call, since sometimes the user's
compiler will link against an older libomptarget. Maybe if we change
the name with offloading in the future we can simplify this.

Depends on #80460

jdoerfert · 2024-02-06T16:33:33Z

openmp/libomptarget/src/OffloadRTL.cpp

@@ -20,25 +20,37 @@
 extern void llvm::omp::target::ompt::connectLibrary();
 #endif

-__attribute__((constructor(101))) void init() {
+static std::mutex PluginMtx;
+static std::atomic<uint32_t> RefCount = 0;


Why is this atomic, it seems all accesses are guarded by PluginMtx?

Thought that myself, was erring on the side of caution in case someone somewhere wants to read it, but likely a non-issue.

Rather put the counter into PM, and wrap PM into ProtectedObj? That makes it clear. I dislike complexity that is not needed, but I get why you went here. Still, this way one thinks the RefCount is accessed concurrently. ProtectedObj has benefits over a standalone Mtx too.

That seems more complicated to me, since we'd need to somehow put the reference count into it, whereas as far as I'm aware the protected object simply wraps a mutex around a single object.

jdoerfert · 2024-02-06T16:35:47Z

openmp/libomptarget/src/OffloadRTL.cpp

-  delete PM;
+void deinitRuntime() {
+  std::scoped_lock<decltype(PluginMtx)> Lock(PluginMtx);
+  if (PM == nullptr)


This seems to be a bad idea as it might hide problems. assert(PM) rather. Maybe we should not even do PM as a pointer? just init/deinit on an object?

Yeah, honestly it's probably correct to just assert that deinit should never be called without initializing it first, rather than handling it. We expect these things to be paired up, so we should assert as such.

jdoerfert · 2024-02-06T16:38:47Z

openmp/libomptarget/test/offloading/runtime_init.c

+
+  // CHECK: PASS
+  printf("PASS\n");
+}


The test is not perfect but I'm unsure how to improve it.

I could add a test that dlopens a bunch of different libraries in parallel or something. Realistically this needs to be a unit test, but we're missing a bit of infrastructure right now. Figured that would come later.

openmp/libomptarget/src/OffloadRTL.cpp

jhuber6 · 2024-02-16T14:57:08Z

ping

JonChesterfield · 2024-02-21T14:41:43Z

It's weird to have a mutex and an atomic reference count. Suggest we drop the mutex and just reference count it. fetch_add or cas into it and finding a zero says you're the first thread to get there.

I think std::mutex has a constructor, in which case you're replacing a global ctor with a global ctor.

JonChesterfield

Refcount it correctly and drop the mutex, much simpler (in terms of runtime behaviour) and removes failure modes (in terms of runtime behaviour)

edit: ^ voted down on that in favour of more complicated runtime behaviour in exchange for more familiar programming constructs

JonChesterfield · 2024-02-21T14:43:39Z

openmp/libomptarget/src/OffloadRTL.cpp

@@ -48,7 +48,8 @@ void deinitRuntime() {
  std::scoped_lock<decltype(PluginMtx)> Lock(PluginMtx);
  assert(PM && "Runtime not initialized");

-  if (RefCount-- == 0) {
+  RefCount--;


This looks wrong - you're separating the decrement from the load, should be a fetch_sub followed by a branch on the old value

JonChesterfield · 2024-02-21T14:56:14Z

I've been persuaded that the rest of the team thinks mutex is much simpler than atomics so we should go with that.

.

dhruvachak

LGTM. The init/deinit code should not be performance critical, so i think having a mutex is ok. Now that RefCount is not atomic, there should not be any confusion about any future usage, it would have to be protected by the mutex.

In addition, I did some testing with OMPT and this patch since OMPT has some ordering constraints. That appeared ok too.

github-actions · 2024-02-22T17:46:09Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Summary: Currently we rely on global constructors to initialize and shut down the OpenMP runtime library and plugin manager. This causes some issues because we do not have a defined lifetime that we can rely on to release and allocate resources. This patch instead adds some simple reference counted initialization and deinitialization function. A future patch will use the `deinit` interface to more intelligently handle plugin deinitilization. Right now we do nothing and rely on `atexit` inside of the plugins to tear them down. This isn't great because it limits our ability to control these things. Note that I made the `__tgt_register_lib` functions do the initialization instead of adding calls to the new runtime functions in the linker wrapper. The reason for this is because in the past it's been easier to not introduce a new function call, since sometimes the user's compiler will link against an older `libomptarget`. Maybe if we change the name with offloading in the future we can simplify this. Depends on llvm#80460 Fix decrement

) Summary: Currently we rely on global constructors to initialize and shut down the OpenMP runtime library and plugin manager. This causes some issues because we do not have a defined lifetime that we can rely on to release and allocate resources. This patch instead adds some simple reference counted initialization and deinitialization function. A future patch will use the `deinit` interface to more intelligently handle plugin deinitilization. Right now we do nothing and rely on `atexit` inside of the plugins to tear them down. This isn't great because it limits our ability to control these things. Note that I made the `__tgt_register_lib` functions do the initialization instead of adding calls to the new runtime functions in the linker wrapper. The reason for this is because in the past it's been easier to not introduce a new function call, since sometimes the user's compiler will link against an older `libomptarget`. Maybe if we change the name with offloading in the future we can simplify this. Depends on llvm#80460 Change-Id: I70815457fab9b5d68db8e48b3b5e1c75951c05f5

jhuber6 requested review from dhruvachak, jdoerfert, JonChesterfield, jplehr, ronlieb and shiltian February 2, 2024 22:13

llvmbot added the openmp:libomptarget OpenMP offload runtime label Feb 2, 2024

jdoerfert reviewed Feb 6, 2024

View reviewed changes

jhuber6 force-pushed the Refcount branch from 692c687 to d26ab1d Compare February 6, 2024 16:54

dhruvachak reviewed Feb 6, 2024

View reviewed changes

openmp/libomptarget/src/OffloadRTL.cpp Outdated Show resolved Hide resolved

JonChesterfield previously requested changes Feb 21, 2024

View reviewed changes

dhruvachak approved these changes Feb 21, 2024

View reviewed changes

jhuber6 force-pushed the Refcount branch 3 times, most recently from 4547c7d to ec4a76a Compare February 22, 2024 17:43

jhuber6 force-pushed the Refcount branch from ec4a76a to e0643a2 Compare February 22, 2024 17:47

jhuber6 merged commit ea174c0 into llvm:main Feb 22, 2024

This was referenced Mar 30, 2024

[OpenMP] Offload incorrectly calls cuda from a static initializer #74507

Closed

[OMPT] Runtime can segfault during at ompt_start_tool when offloading and calling std:: functions #69318

Closed

[Libomptarget] Remove global ctor and use reference counting #80499

[Libomptarget] Remove global ctor and use reference counting #80499

Uh oh!

Conversation

jhuber6 commented Feb 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhuber6 commented Feb 16, 2024

Uh oh!

JonChesterfield commented Feb 21, 2024

Uh oh!

JonChesterfield left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonChesterfield commented Feb 21, 2024

Uh oh!

dhruvachak left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JonChesterfield left a comment •

edited

Loading

github-actions bot commented Feb 22, 2024 •

edited

Loading