-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[libclc] Add OpenCL atomic_*_explicit builtins #168318
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
Conversation
Implement atomic_*_explicit (e.g. atomic_store_explicit) with memory_order plus optional memory_scope. OpenCL memory_order maps 1:1 to Clang (e.g. OpenCL memory_order_relaxed == Clang __ATOMIC_RELAXED), so we pass it unchanged to clc_atomic_* function which forwards to Clang _scoped_atomic* builtins. Other changes: * Add __opencl_get_clang_memory_scope helper in opencl/utils.h (OpenCL scope -> Clang scope). * Correct atomic_compare_exchange return type to bool. * Fix atomic_compare_exchange to return true when value stored in the pointer equals expected value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements OpenCL atomic operations with explicit memory ordering and scope control. The key changes add support for atomic_*_explicit variants (e.g., atomic_store_explicit) that accept memory_order and optional memory_scope parameters, aligning with OpenCL's atomic API requirements.
Key changes:
- Implements
atomic_*_explicitbuiltins with memory_order and memory_scope parameters - Adds helper function to convert OpenCL memory scopes to Clang memory scopes
- Corrects atomic_compare_exchange return type from generic type to bool
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| libclc/opencl/include/clc/opencl/utils.h | New utility header defining __opencl_get_clang_memory_scope for scope conversion |
| libclc/opencl/include/clc/opencl/types.h | New header defining OpenCL memory_order and memory_scope enums |
| libclc/opencl/lib/generic/atomic/atomic_def.inc | Implements three tiers of atomic functions: explicit with scope, explicit without scope, and implicit seq_cst |
| libclc/opencl/include/clc/opencl/atomic/atomic_decl.inc | Adds function declarations for all three tiers of atomic operations |
| Various atomic header/implementation files | Removes old conditional compilation guards and adds includes for new utility headers |
Comments suppressed due to low confidence (1)
libclc/opencl/lib/generic/atomic/atomic_def.inc:1
- The compare_exchange_explicit function signature accepts separate Success and Failure memory orders, but the declaration in atomic_decl.inc at line 45 only declares a single 'Order' parameter for the version without __opencl_c_atomic_scope_device guard. This API inconsistency will cause compilation errors.
//===----------------------------------------------------------------------===//
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
kindly ping |
| #ifndef __CLC_OPENCL_TYPES_H__ | ||
| #define __CLC_OPENCL_TYPES_H__ | ||
|
|
||
| // Copied from clang/lib/Headers/opencl-c-base.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't need copying? Use the builtin included declarations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't need copying? Use the builtin included declarations?
The copying is deleted. e3799b2 added -include opencl-c-base.h to compile flags for .cl files. Some types and constants in libclc are not needed anymore.
opencl-c-base.h only defines OpenCL types and constant, so it should be safe to use in libclc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't need copying? Use the builtin included declarations?
The copying is deleted. e3799b2 added
-include opencl-c-base.hto compile flags for .cl files. Some types and constants in libclc are not needed anymore. opencl-c-base.h only defines OpenCL types and constant, so it should be safe to use in libclc.
I think we can also change OpenCL enum memory_scope to map 1:1 to Clang memory_scope like in following diff:
diff --git a/clang/lib/Headers/opencl-c-base.h b/clang/lib/Headers/opencl-c-base.h
index 898026c66614..50c6c4ed3d27 100644
--- a/clang/lib/Headers/opencl-c-base.h
+++ b/clang/lib/Headers/opencl-c-base.h
@@ -295,13 +295,13 @@ typedef uint cl_mem_fence_flags;
#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
typedef enum memory_scope {
- memory_scope_work_item = __OPENCL_MEMORY_SCOPE_WORK_ITEM,
- memory_scope_work_group = __OPENCL_MEMORY_SCOPE_WORK_GROUP,
- memory_scope_device = __OPENCL_MEMORY_SCOPE_DEVICE,
+ memory_scope_work_item = __MEMORY_SCOPE_SINGLE,
+ memory_scope_work_group = __MEMORY_SCOPE_WRKGRP,
+ memory_scope_device = __MEMORY_SCOPE_DEVICE,
#if defined(__opencl_c_atomic_scope_all_devices)
- memory_scope_all_svm_devices = __OPENCL_MEMORY_SCOPE_ALL_SVM_DEVICES,
+ memory_scope_all_svm_devices = __MEMORY_SCOPE_SYSTEM,
#if (__OPENCL_C_VERSION__ >= CL_VERSION_3_0 || __OPENCL_CPP_VERSION__ >= 202100)
- memory_scope_all_devices = memory_scope_all_svm_devices,
+ memory_scope_all_devices = __MEMORY_SCOPE_SYSTEM,
#endif // (__OPENCL_C_VERSION__ >= CL_VERSION_3_0 || __OPENCL_CPP_VERSION__ >= 202100)
#endif // defined(__opencl_c_atomic_scope_all_devices)
/**
@@ -311,7 +311,7 @@ typedef enum memory_scope {
* KHR subgroups "Subgroups within a workgroup are independent, make forward progress with respect to each other"
*/
#if defined(cl_intel_subgroups) || defined(cl_khr_subgroups) || defined(__opencl_c_subgroups)
- memory_scope_sub_group = __OPENCL_MEMORY_SCOPE_SUB_GROUP
+ memory_scope_sub_group = __MEMORY_SCOPE_WVFRNT
#endif
} memory_scope;
Then we don't need to include opencl-c-base.h for libclc. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we don't need to include opencl-c-base.h for libclc.
sorry, -include opencl-c-base.h is still needed. The above mapping change can only eliminates helper function __opencl_get_clang_memory_scope.
…efinitions in clc
|
@arsenm could you please take a look again? thanks. This is blocking our downstream development. |
|
kindly ping |
libclc/cmake/modules/AddLibclc.cmake
Outdated
| if( NOT ${FILE_EXT} STREQUAL ".ll" ) | ||
| # Pass '-c' when not running the preprocessor | ||
| set( PP_OPTS -c ) | ||
| set( PP_OPTS -c -include opencl-c-base.h ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should be using -finclude-default-header or -fdeclare-opencl-builtin instead?
I'm assuming this picks up the clang builtin copy, not the duplicated name one in libclc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should be using -finclude-default-header or -fdeclare-opencl-builtin instead?
I'm assuming this picks up the clang builtin copy, not the duplicated name one in libclc?
Done, changed to -Xclang -fdeclare-opencl-builtins -Xclang -finclude-default-header in 58ddca5
llvm-diff shows the commit causes no change to amdgcn--amdhsa.bc and nvptx64--nvidiacl.bc.
The flag will pull in builtin declarations from clang, so the OpenCL builtin declarations in libclc are not needed anymore. I'll remove them in a separate PR.
This is follow-up of comment llvm#168318 (comment) libclc OpenCL library is already compiled with flag `-fdeclare-opencl-builtins -finclude-default-header`.
This is follow-up of comment #168318 (comment) libclc OpenCL library is already compiled with flag `-fdeclare-opencl-builtins -finclude-default-header`.
This is follow-up of comment llvm/llvm-project#168318 (comment) libclc OpenCL library is already compiled with flag `-fdeclare-opencl-builtins -finclude-default-header`.
Implement atomic_*_explicit (e.g. atomic_store_explicit) with memory_order plus optional memory_scope. OpenCL memory_order maps 1:1 to Clang (e.g. OpenCL memory_order_relaxed == Clang __ATOMIC_RELAXED), so we pass it unchanged to clc_atomic_* function which forwards to Clang _scoped_atomic* builtins. Other changes: * Add __opencl_get_clang_memory_scope helper in opencl/utils.h (OpenCL scope -> Clang scope). * Correct atomic_compare_exchange return type to bool. * Fix atomic_compare_exchange to return true when value stored in the pointer equals expected value. * Remove volatile from CLC functions so that volatile isn't present in LLVM IR. * Add '-fdeclare-opencl-builtins -finclude-default-header' flag to include declaration of memory_scope. Some constants in libclc are already provided by Clang’s OpenCL header; disable those in OpenCL library build and enable them only for CLC library build.
This is follow-up of comment llvm#168318 (comment) libclc OpenCL library is already compiled with flag `-fdeclare-opencl-builtins -finclude-default-header`.
Implement atomic_*_explicit (e.g. atomic_store_explicit) with memory_order plus optional memory_scope.
OpenCL memory_order maps 1:1 to Clang (e.g. OpenCL memory_order_relaxed == Clang _ATOMIC_RELAXED), so we pass it unchanged to clc_atomic* function which forwards to Clang _scoped_atomic* builtins.
Other changes:
declaration of memory_scope. Some constants in libclc are already provided
by Clang’s OpenCL header; disable those in OpenCL library build and
enable them only for CLC library build.