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

[Clang][OpenCL] Fix wait_for_event argument address space with -fdeclare-opencl-builtins #134598

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

jmmartinez
Copy link
Contributor

The pointer argument for wait_for_event(int, event_t*) should take the default address space: generic if available, otherwise private.

Before this patch it would always be generic with -fdeclare-opencl-builtins. This was inconsistent with the behavior when opencl-c.h is included.

…cl-builtins

The pointer argument of the wait_for_event builtin should take the
default address space: generic if available, otherwise private.
@jmmartinez jmmartinez requested a review from svenvh April 7, 2025 08:23
@jmmartinez jmmartinez self-assigned this Apr 7, 2025
@jmmartinez jmmartinez changed the title [Clang] Fix wait_for_event argument address space with -fdeclare-opencl-builtins [Clang][OpenCL] Fix wait_for_event argument address space with -fdeclare-opencl-builtins Apr 7, 2025
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 7, 2025

@llvm/pr-subscribers-clang

Author: Juan Manuel Martinez Caamaño (jmmartinez)

Changes

The pointer argument for wait_for_event(int, event_t*) should take the default address space: generic if available, otherwise private.

Before this patch it would always be generic with -fdeclare-opencl-builtins. This was inconsistent with the behavior when opencl-c.h is included.


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

2 Files Affected:

  • (modified) clang/lib/Sema/OpenCLBuiltins.td (+15-3)
  • (modified) clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl (+9)
diff --git a/clang/lib/Sema/OpenCLBuiltins.td b/clang/lib/Sema/OpenCLBuiltins.td
index 4da61429fcce7..528b700a275e0 100644
--- a/clang/lib/Sema/OpenCLBuiltins.td
+++ b/clang/lib/Sema/OpenCLBuiltins.td
@@ -958,13 +958,25 @@ foreach name = ["async_work_group_strided_copy"] in {
   def : Builtin<name, [Event, PointerType<AGenTypeN, LocalAS>, PointerType<ConstType<AGenTypeN>, GlobalAS>, Size, Size, Event]>;
   def : Builtin<name, [Event, PointerType<AGenTypeN, GlobalAS>, PointerType<ConstType<AGenTypeN>, LocalAS>, Size, Size, Event]>;
 }
-foreach name = ["wait_group_events"] in {
-  def : Builtin<name, [Void, Int, PointerType<Event, GenericAS>]>;
-}
 foreach name = ["prefetch"] in {
   def : Builtin<name, [Void, PointerType<ConstType<AGenTypeN>, GlobalAS>, Size]>;
 }
 
+// The wait_group_events is declared with an argument of type event_t*.
+// The address-space of the pointer parameter is different if the generic address space is available.
+multiclass BuiltinWithDefaultPointerArg<AddressSpace AS> {
+  foreach name = ["wait_group_events"] in {
+    def : Builtin<name, [Void, Int, PointerType<Event, AS>]>;
+  }
+}
+
+let Extension = FuncExtOpenCLCNamedAddressSpaceBuiltins in {
+  defm : BuiltinWithDefaultPointerArg<PrivateAS>;
+}
+let Extension = FuncExtOpenCLCGenericAddressSpace in {
+  defm : BuiltinWithDefaultPointerArg<GenericAS>;
+}
+
 //--------------------------------------------------------------------
 // OpenCL v2.0 s6.13.11 - Atomics Functions.
 // Functions that use memory_order and cl_mem_fence_flags enums are not
diff --git a/clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl b/clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
index ac3bff9dbde27..8bd1db5d06819 100644
--- a/clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
+++ b/clang/test/CodeGenOpenCL/fdeclare-opencl-builtins.cl
@@ -48,6 +48,15 @@ void test_generic_optionality(float a, float *b) {
   float res = fract(a, b);
 }
 
+// Test that the correct builtin is called depending on the generic address
+// space feature availability. If not available, the __private version is called
+// CHECK-LABEL: @test_wait_group_events
+// CHECK-GAS: call spir_func void @_Z17wait_group_eventsiPU3AS49ocl_event
+// CHECK-NOGAS: call spir_func void @_Z17wait_group_eventsiP9ocl_event
+void test_wait_group_events(int i, event_t *e) {
+  wait_group_events(i, e);
+}
+
 // CHECK: attributes [[ATTR_CONST]] =
 // CHECK-SAME: memory(none)
 // CHECK: attributes [[ATTR_PURE]] =

Copy link
Member

@svenvh svenvh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

@jmmartinez jmmartinez merged commit 44e32fb into llvm:main Apr 7, 2025
14 checks passed
@jmmartinez
Copy link
Contributor Author

Thanks for the quick review !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants