Skip to content

[libc] Remove use of BlockStore for GPU atexit #82823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Feb 23, 2024

Summary:
The GPU backends have restrictions on the kinds of initializers they can
produce. The use of BlockStore here currently breaks the backends
through the use of recursive initializers. This prevents it from
actually being included in any builds. This patchs changes it to just
use a fixed size of 64 slots .The chances of someone exceeding the 64
slots in practice is very, very low.

However, this is primarily a bandaid solution as a real solution will
need to use a lock free data structure to push work in parallel.
Currently the mutexes on the GPU build do nothing, so they only work if
the user guards the use themselves.

Summary:
The GPU backends have restrictions on the kinds of initializers they can
produce. The use of BlockStore here currently breaks the backends
through the use of recursive initializers. This prevents it from
actually being included in any builds. This patchs changes it to just
use a fixed size of 64 slots .The chances of someone exceeding the 64
slots in practice is very, very low.

However, this is primarily a bandaid solution as a real solution will
need to use a lock free data structure to push work in parallel.
Currently the mutexes on the GPU build do nothing, so they only work if
the user guards the use themselves.
@llvmbot
Copy link
Member

llvmbot commented Feb 23, 2024

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
The GPU backends have restrictions on the kinds of initializers they can
produce. The use of BlockStore here currently breaks the backends
through the use of recursive initializers. This prevents it from
actually being included in any builds. This patchs changes it to just
use a fixed size of 64 slots .The chances of someone exceeding the 64
slots in practice is very, very low.

However, this is primarily a bandaid solution as a real solution will
need to use a lock free data structure to push work in parallel.
Currently the mutexes on the GPU build do nothing, so they only work if
the user guards the use themselves.


Full diff: https://github.com/llvm/llvm-project/pull/82823.diff

1 Files Affected:

  • (modified) libc/src/stdlib/atexit.cpp (+8-1)
diff --git a/libc/src/stdlib/atexit.cpp b/libc/src/stdlib/atexit.cpp
index 10dff42b1be925..1513b7969f0dbc 100644
--- a/libc/src/stdlib/atexit.cpp
+++ b/libc/src/stdlib/atexit.cpp
@@ -28,7 +28,14 @@ struct AtExitUnit {
   constexpr AtExitUnit(AtExitCallback *c, void *p) : callback(c), payload(p) {}
 };
 
-#ifdef LIBC_COPT_PUBLIC_PACKAGING
+#if defined(LIBC_TARGET_ARCH_IS_GPU)
+// The GPU build cannot handle the potentially recursive definitions required by
+// the BlockStore class. Additionally, the liklihood that someone exceeds this
+// while executing on the GPU is extremely small.
+// FIXME: It is not generally safe to use 'atexit' on the GPU because the
+//        mutexes simply passthrough. We will need a lock free stack.
+using ExitCallbackList = FixedVector<AtExitUnit, 64>;
+#elif defined(LIBC_COPT_PUBLIC_PACKAGING)
 using ExitCallbackList = cpp::ReverseOrderBlockStore<AtExitUnit, 32>;
 #else
 // BlockStore uses dynamic memory allocation. To avoid dynamic memory

Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jhuber6 jhuber6 merged commit a3a316e into llvm:main Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants