Skip to content
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

[DeviceRTL] Make defined 'libc' functions weak in OpenMP #97356

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Jul 1, 2024

Summary:
These functions provide special-case implementations internal to the
OpenMP device runtime. This can potentially conflict with the symbols
pulled in from the actual GPU libc. This patch makes these weak, so in
the case that the GPU libc functions exist they will be overridden. This
should not impact performance in the average case because the old
-mlink-builtin-bitcode version does internalization, deleting weak,
and the new LTO path will resolve to the strong reference and then
internalize it.

Summary:
These functions provide special-case implementations internal to the
OpenMP device runtime. This can potentially conflict with the symbols
pulled in from the actual GPU `libc`. This patch makes these weak, so in
the case that the GPU libc functions exist they will be overridden. This
should not impact performance in the average case because the old
`-mlink-builtin-bitcode` version does internalization, deleting weak,
and the new LTO path will resolve to the strong reference and then
internalize it.
@llvmbot
Copy link
Member

llvmbot commented Jul 1, 2024

@llvm/pr-subscribers-offload

Author: Joseph Huber (jhuber6)

Changes

Summary:
These functions provide special-case implementations internal to the
OpenMP device runtime. This can potentially conflict with the symbols
pulled in from the actual GPU libc. This patch makes these weak, so in
the case that the GPU libc functions exist they will be overridden. This
should not impact performance in the average case because the old
-mlink-builtin-bitcode version does internalization, deleting weak,
and the new LTO path will resolve to the strong reference and then
internalize it.


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

2 Files Affected:

  • (modified) offload/DeviceRTL/src/Debug.cpp (+2-2)
  • (modified) offload/DeviceRTL/src/LibC.cpp (+2-2)
diff --git a/offload/DeviceRTL/src/Debug.cpp b/offload/DeviceRTL/src/Debug.cpp
index 31cd54e3de35c..4e16591cc6c51 100644
--- a/offload/DeviceRTL/src/Debug.cpp
+++ b/offload/DeviceRTL/src/Debug.cpp
@@ -26,8 +26,8 @@ using namespace ompx;
 extern "C" {
 void __assert_assume(bool condition) { __builtin_assume(condition); }
 
-void __assert_fail(const char *expr, const char *file, unsigned line,
-                   const char *function) {
+[[gnu::weak]] void __assert_fail(const char *expr, const char *file,
+                                 unsigned line, const char *function) {
   __assert_fail_internal(expr, nullptr, file, line, function);
 }
 void __assert_fail_internal(const char *expr, const char *msg, const char *file,
diff --git a/offload/DeviceRTL/src/LibC.cpp b/offload/DeviceRTL/src/LibC.cpp
index e587c3057f5ba..4bca5d29643fe 100644
--- a/offload/DeviceRTL/src/LibC.cpp
+++ b/offload/DeviceRTL/src/LibC.cpp
@@ -49,7 +49,7 @@ int32_t omp_vprintf(const char *Format, void *Arguments, uint32_t) {
 
 extern "C" {
 
-int memcmp(const void *lhs, const void *rhs, size_t count) {
+[[gnu::weak]] int memcmp(const void *lhs, const void *rhs, size_t count) {
   auto *L = reinterpret_cast<const unsigned char *>(lhs);
   auto *R = reinterpret_cast<const unsigned char *>(rhs);
 
@@ -60,7 +60,7 @@ int memcmp(const void *lhs, const void *rhs, size_t count) {
   return 0;
 }
 
-void memset(void *dst, int C, size_t count) {
+[[gnu::weak]] void memset(void *dst, int C, size_t count) {
   auto *dstc = reinterpret_cast<char *>(dst);
   for (size_t I = 0; I < count; ++I)
     dstc[I] = C;

Copy link
Member

@jdoerfert jdoerfert left a comment

Choose a reason for hiding this comment

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

Can we have a test?

Copy link
Member

@jdoerfert jdoerfert left a comment

Choose a reason for hiding this comment

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

Assuming this doesn't impact our optimization for the "default path", LGTM.

@jhuber6 jhuber6 merged commit 3c50cbf into llvm:main Jul 2, 2024
7 checks passed
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
Summary:
These functions provide special-case implementations internal to the
OpenMP device runtime. This can potentially conflict with the symbols
pulled in from the actual GPU `libc`. This patch makes these weak, so in
the case that the GPU libc functions exist they will be overridden. This
should not impact performance in the average case because the old
`-mlink-builtin-bitcode` version does internalization, deleting weak,
and the new LTO path will resolve to the strong reference and then
internalize it.
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
Summary:
These functions provide special-case implementations internal to the
OpenMP device runtime. This can potentially conflict with the symbols
pulled in from the actual GPU `libc`. This patch makes these weak, so in
the case that the GPU libc functions exist they will be overridden. This
should not impact performance in the average case because the old
`-mlink-builtin-bitcode` version does internalization, deleting weak,
and the new LTO path will resolve to the strong reference and then
internalize it.
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