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

[NVPTX][NFC] Change naming for global objects for ctor / dtor handling #72137

Closed
wants to merge 1 commit into from

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Nov 13, 2023

Summary:
To mimic standard ctor / dtor handling, we emit a bunch of globals for
function pointers that were marked as being a constructor or destructor.
These were previously emitted with __ syntax. Realistically, we want
symbol names that are not targetable from user code so they cannot
conflict. Normally, this is done with ., but NVPTX cannot support
these names. This patch changes it to use $ which seems to be the
NVPTX equivalent for "This is an internal symbol".

Summary:
To mimic standard ctor / dtor handling, we emit a bunch of globals for
function pointers that were marked as being a constructor or destructor.
These were previously emitted with `__` syntax. Realistically, we want
symbol names that are not targetable from user code so they cannot
conflict. Normally, this is done with `.`, but NVPTX cannot support
these names. This patch changes it to use `$` which seems to be the
NVPTX equivalent for "This is an internal symbol".
@jhuber6 jhuber6 requested a review from Artem-B November 13, 2023 17:04
@llvmbot llvmbot added libc openmp:libomptarget OpenMP offload runtime labels Nov 13, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 13, 2023

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
To mimic standard ctor / dtor handling, we emit a bunch of globals for
function pointers that were marked as being a constructor or destructor.
These were previously emitted with __ syntax. Realistically, we want
symbol names that are not targetable from user code so they cannot
conflict. Normally, this is done with ., but NVPTX cannot support
these names. This patch changes it to use $ which seems to be the
NVPTX equivalent for "This is an internal symbol".


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

3 Files Affected:

  • (modified) libc/utils/gpu/loader/nvptx/Loader.cpp (+4-4)
  • (modified) llvm/test/CodeGen/NVPTX/lower-ctor-dtor.ll (+10-10)
  • (modified) openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp (+3-3)
diff --git a/libc/utils/gpu/loader/nvptx/Loader.cpp b/libc/utils/gpu/loader/nvptx/Loader.cpp
index 5388f287063b7f9..1370fcc60e9790f 100644
--- a/libc/utils/gpu/loader/nvptx/Loader.cpp
+++ b/libc/utils/gpu/loader/nvptx/Loader.cpp
@@ -66,15 +66,15 @@ Expected<void *> get_ctor_dtor_array(const void *image, const size_t size,
       handle_error(toString(name_or_err.takeError()).c_str());
 
     // Search for all symbols that contain a constructor or destructor.
-    if (!name_or_err->starts_with("__init_array_object_") &&
-        !name_or_err->starts_with("__fini_array_object_"))
+    if (!name_or_err->starts_with("$init_array_object$") &&
+        !name_or_err->starts_with("$fini_array_object$"))
       continue;
 
     uint16_t priority;
-    if (name_or_err->rsplit('_').second.getAsInteger(10, priority))
+    if (name_or_err->rsplit('$').second.getAsInteger(10, priority))
       handle_error("Invalid priority for constructor or destructor");
 
-    if (name_or_err->starts_with("__init"))
+    if (name_or_err->starts_with("$init"))
       ctors.emplace_back(std::make_pair(name_or_err->data(), priority));
     else
       dtors.emplace_back(std::make_pair(name_or_err->data(), priority));
diff --git a/llvm/test/CodeGen/NVPTX/lower-ctor-dtor.ll b/llvm/test/CodeGen/NVPTX/lower-ctor-dtor.ll
index f8b3b4b9b8c4467..6e636b9918a4fe7 100644
--- a/llvm/test/CodeGen/NVPTX/lower-ctor-dtor.ll
+++ b/llvm/test/CodeGen/NVPTX/lower-ctor-dtor.ll
@@ -17,23 +17,23 @@
 ; CHECK-NOT: @llvm.global_ctors
 ; CHECK-NOT: @llvm.global_dtors
 
-; CHECK: @__init_array_object_foo_[[HASH:[0-9a-f]+]]_1 = protected addrspace(4) constant ptr @foo, section ".init_array.1"
-; CHECK: @__fini_array_object_bar_[[HASH:[0-9a-f]+]]_1 = protected addrspace(4) constant ptr @bar, section ".fini_array.1"
-; CHECK: @llvm.used = appending global [2 x ptr] [ptr addrspacecast (ptr addrspace(4) @__init_array_object_foo_[[HASH]]_1 to ptr), ptr addrspacecast (ptr addrspace(4) @__fini_array_object_bar_[[HASH]]_1 to ptr)], section "llvm.metadata"
+; CHECK: @"$init_array_object$foo$[[HASH:[0-9a-f]+]]$1" = protected addrspace(4) constant ptr @foo, section ".init_array.1"
+; CHECK: @"$fini_array_object$bar$[[HASH:[0-9a-f]+]]$1" = protected addrspace(4) constant ptr @bar, section ".fini_array.1"
+; CHECK: @llvm.used = appending global [2 x ptr] [ptr addrspacecast (ptr addrspace(4) @"$init_array_object$foo$[[HASH]]$1" to ptr), ptr addrspacecast (ptr addrspace(4) @"$fini_array_object$bar$[[HASH]]$1" to ptr)], section "llvm.metadata"
 ; CHECK: @__fini_array_start = weak protected addrspace(1) global ptr null
 ; CHECK: @__fini_array_end = weak protected addrspace(1) global ptr null
 
-; GLOBAL: @__init_array_object_foo_unique_id_1 = protected addrspace(4) constant ptr @foo, section ".init_array.1"
-; GLOBAL: @__fini_array_object_bar_unique_id_1 = protected addrspace(4) constant ptr @bar, section ".fini_array.1"
-; GLOBAL: @llvm.used = appending global [2 x ptr] [ptr addrspacecast (ptr addrspace(4) @__init_array_object_foo_unique_id_1 to ptr), ptr addrspacecast (ptr addrspace(4) @__fini_array_object_bar_unique_id_1 to ptr)], section "llvm.metadata"
+; GLOBAL: @"$init_array_object$foo$unique_id$1" = protected addrspace(4) constant ptr @foo, section ".init_array.1"
+; GLOBAL: @"$fini_array_object$bar$unique_id$1" = protected addrspace(4) constant ptr @bar, section ".fini_array.1"
+; GLOBAL: @llvm.used = appending global [2 x ptr] [ptr addrspacecast (ptr addrspace(4) @"$init_array_object$foo$unique_id$1" to ptr), ptr addrspacecast (ptr addrspace(4) @"$fini_array_object$bar$unique_id$1" to ptr)], section "llvm.metadata"
 ; GLOBAL: @__fini_array_start = weak protected addrspace(1) global ptr null
 ; GLOBAL: @__fini_array_end = weak protected addrspace(1) global ptr null
 
-; KERNEL: @__init_array_object_foo_[[HASH:[0-9a-f]+]]_1 = protected addrspace(4) constant ptr @foo, section ".init_array.1"
-; KERNEL: @__fini_array_object_bar_[[HASH:[0-9a-f]+]]_1 = protected addrspace(4) constant ptr @bar, section ".fini_array.1"
+; KERNEL: @"$init_array_object$foo$[[HASH:[0-9a-f]+]]$1" = protected addrspace(4) constant ptr @foo, section ".init_array.1"
+; KERNEL: @"$fini_array_object$bar$[[HASH:[0-9a-f]+]]$1" = protected addrspace(4) constant ptr @bar, section ".fini_array.1"
 
-; VISIBILITY: .visible .const .align 8 .u64 __init_array_object_foo_[[HASH:[0-9a-f]+]]_1 = foo;
-; VISIBILITY: .visible .const .align 8 .u64 __fini_array_object_bar_[[HASH:[0-9a-f]+]]_1 = bar;
+; VISIBILITY: .visible .const .align 8 .u64 $init_array_object$foo$[[HASH:[0-9a-f]+]]$1 = foo;
+; VISIBILITY: .visible .const .align 8 .u64 $fini_array_object$bar$[[HASH:[0-9a-f]+]]$1 = bar;
 
 define internal void @foo() {
   ret void
diff --git a/openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp b/openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp
index a6e28574a7f08e3..1d9425caa1ae45b 100644
--- a/openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp
+++ b/openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp
@@ -1077,12 +1077,12 @@ struct CUDADeviceTy : public GenericDeviceTy {
       if (!NameOrErr)
         return NameOrErr.takeError();
 
-      if (!NameOrErr->starts_with(IsCtor ? "__init_array_object_"
-                                         : "__fini_array_object_"))
+      if (!NameOrErr->starts_with(IsCtor ? "$init_array_object$"
+                                         : "$fini_array_object$"))
         continue;
 
       uint16_t Priority;
-      if (NameOrErr->rsplit('_').second.getAsInteger(10, Priority))
+      if (NameOrErr->rsplit('$').second.getAsInteger(10, Priority))
         return Plugin::error("Invalid priority for constructor or destructor");
 
       Funcs.emplace_back(*NameOrErr, Priority);

@Artem-B
Copy link
Member

Artem-B commented Nov 13, 2023

not targetable from user code

But it is. https://godbolt.org/z/eE3d6raYd

What's worse, I think unlike double-underscore symbols that are explicitly reserved for compiler's own use, we do not have such conventions for $-prefixed ones. Yes, they may be uncommon, but that's not a very convincing argument that the change will be an improvement over double-underscore prefix.

@jhuber6
Copy link
Contributor Author

jhuber6 commented Nov 13, 2023

not targetable from user code

But it is. https://godbolt.org/z/eE3d6raYd

What's worse, I think unlike double-underscore symbols that are explicitly reserved for compiler's own use, we do not have such conventions for $-prefixed ones. Yes, they may be uncommon, but that's not a very convincing argument that the change will be an improvement over double-underscore prefix.

I actually never knew you could actually use $ in C identifiers. I think I just assumed it wasn't allowed from looking at code like https://github.com/llvm/llvm-project/blob/main/lld/Common/Strings.cpp#L66 in the past which doesn't seem to consider $. Seems to be a reverse problem of PTX https://gcc.gnu.org/onlinedocs/gcc/Dollar-Signs.html. So it's one of those things that's technically permitted but likely to break. Good to know I guess.

@jhuber6 jhuber6 closed this Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc openmp:libomptarget OpenMP offload runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants