Skip to content

[libclc] Add generic implementation of some atomic functions in OpenCL spec section 6.15.12.7 #146814

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 9 commits into from
Jul 18, 2025

Conversation

wenju-he
Copy link
Contributor

@wenju-he wenju-he commented Jul 3, 2025

Add corresponding clc functions, which are implemented with clang __scoped_atomic builtins. OpenCL functions are implemented as a wrapper over clc functions.

Also change legacy atomic_inc and atomic_dec to re-use the newly added clc_atomic_inc/dec implementations. llvm-diff only no change to atomic_inc and atomic_dec in bitcode.

Notes:

  • Generic OpenCL built-ins functions uses __ATOMIC_SEQ_CST and __MEMORY_SCOPE_DEVICE for memory order and memory scope parameters.
  • OpenCL atomic__explicit, atomic_flag built-ins are not implemented yet.
  • OpenCL built-ins of atomic_intptr_t, atomic_uintptr_t, atomic_size_t and atomic_ptrdiff_t types are not implemented yet.
  • llvm-diff shows no change to nvptx64--nvidiacl.bc and amdgcn--amdhsa.bc since __opencl_c_atomic_order_seq_cst and __opencl_c_atomic_scope_device are not defined in these two targets.

…L spec section 6.15.12.7

Add corresponding clc functions, which are implemented with clang
__scoped_atomic builtins. OpenCL functions are implemented as a wrapper
over clc functions.

Also change legacy atomic_inc and atomic_dec to re-use the newly added
clc_atomic_inc/dec implementations. llvm-diff only no change to
atomic_inc and atomic_dec in bitcode.

Notes:
* Generic OpenCL built-ins functions uses __ATOMIC_SEQ_CST and
  __MEMORY_SCOPE_SYSTEM for memory order and memory scope parameters.
  Ideally we should check if __opencl_c_atomic_order_seq_cst,
  __opencl_c_atomic_scope_all_devices and __opencl_c_atomic_scope_device
  feature macros should be checked. However, none of them are defined for
  nvptx64 and amdgcn arch yet. We can add the check when the backends
  are fixed.
* OpenCL atomic_*_explicit, atomic_flag* built-ins are not implemented yet.
* OpenCL built-ins of atomic_intptr_t, atomic_uintptr_t, atomic_size_t
  and atomic_ptrdiff_t types are not implemented yet.
* llvm-diff only shows new built-ins are added to nvptx64--nvidiacl.bc
  and amdgcn--amdhsa.bc. The number of newly added built-ins in
  amdgcn--amdhsa.bc is higher than in nvptx64--nvidiacl.bc because
  64-bit atomics are enabled for amdgcn--amdhsa.
@wenju-he wenju-he requested a review from frasercrmck July 3, 2025 02:18
wenju-he added 2 commits July 10, 2025 20:00
…c_atomic_scope_device macro is required for the new built-ins
@wenju-he wenju-he requested a review from frasercrmck July 11, 2025 03:18
Copy link
Contributor

@frasercrmck frasercrmck 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 do anything about the warnings I'm seeing while building this? They're noisy.

In file included from /llvm-project/libclc/clc/lib/generic/atomic/clc_atomic_fetch_min.cl:18:
In file included from /llvm-project/libclc/clc/include/clc/math/gentype.inc:239:
/llvm-project/libclc/clc/lib/generic/atomic/atomic_def.inc:59:1: warning: large atomic operation may incur significant performance penalty; the access size (2 bytes) exceeds the max lock-free size (0 bytes) [-Watomic-alignment]
   59 | __CLC_DEFINE_ATOMIC()
      | ^
/llvm-project/libclc/clc/lib/generic/atomic/atomic_def.inc:51:9: note: expanded from macro '__CLC_DEFINE_ATOMIC'
   51 |         __IMPL_FUNCTION((ADDRSPACE __CLC_PTR_CASTTYPE *)Ptr, Value,            \
      |         ^
/llvm-project/libclc/clc/lib/generic/atomic/clc_atomic_fetch_min.cl:12:25: note: expanded from macro '__IMPL_FUNCTION'
   12 | #define __IMPL_FUNCTION __scoped_atomic_fetch_min
      |                         ^

wenju-he added a commit to wenju-he/llvm-project that referenced this pull request Jul 16, 2025
…for spir64

Set MaxAtomicInlineWidth the same way as SPIR-V targets in 3cfd0c0.
This PR fixes build warning in scoped atomic built-in in llvm#146814:
`warning: large atomic operation may incur significant performance penalty`
@wenju-he
Copy link
Contributor Author

Can we do anything about the warnings I'm seeing while building this? They're noisy.

In file included from /llvm-project/libclc/clc/lib/generic/atomic/clc_atomic_fetch_min.cl:18:
In file included from /llvm-project/libclc/clc/include/clc/math/gentype.inc:239:
/llvm-project/libclc/clc/lib/generic/atomic/atomic_def.inc:59:1: warning: large atomic operation may incur significant performance penalty; the access size (2 bytes) exceeds the max lock-free size (0 bytes) [-Watomic-alignment]
   59 | __CLC_DEFINE_ATOMIC()
      | ^
/llvm-project/libclc/clc/lib/generic/atomic/atomic_def.inc:51:9: note: expanded from macro '__CLC_DEFINE_ATOMIC'
   51 |         __IMPL_FUNCTION((ADDRSPACE __CLC_PTR_CASTTYPE *)Ptr, Value,            \
      |         ^
/llvm-project/libclc/clc/lib/generic/atomic/clc_atomic_fetch_min.cl:12:25: note: expanded from macro '__IMPL_FUNCTION'
   12 | #define __IMPL_FUNCTION __scoped_atomic_fetch_min
      |                         ^

thanks @frasercrmck
It seems the warning only exists for clspv target. I have created PR #148997 to fix the warning.

wenju-he added a commit that referenced this pull request Jul 17, 2025
…for spir64 (#148997)

Set MaxAtomicInlineWidth the same way as SPIR-V targets in 3cfd0c0.
This PR fixes build warning in scoped atomic built-in in #146814:
`warning: large atomic operation may incur significant performance
penalty; ; the access size (2 bytes) exceeds the max lock-free size (0
bytes) [-Watomic-alignment]`
wenju-he added 2 commits July 16, 2025 18:03
…ning 'large atomic operation may incur significant performance penalty'

Also rename clc/lib/generic/atomic/atomic_def.inc to clc_atomic_def.inc
@wenju-he
Copy link
Contributor Author

wenju-he commented Jul 17, 2025

@frasercrmck please review new two changes in c88bf9b:

  • clc: disable 64bit atomic for 32bit spir and nvptx targets to fix warning 'large atomic operation may incur significant performance penalty'
  • Also rename clc/lib/generic/atomic/atomic_def.inc to clc_atomic_def.inc

@wenju-he wenju-he requested a review from frasercrmck July 17, 2025 02:35
@wenju-he wenju-he merged commit 9c26f37 into llvm:main Jul 18, 2025
9 checks passed
@wenju-he wenju-he deleted the libclc-atomic-clc branch July 18, 2025 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants