Skip to content

Conversation

@wenju-he
Copy link
Contributor

No description provided.

@wenju-he wenju-he requested a review from Copilot November 17, 2025 07:43
@llvmbot llvmbot added the libclc libclc OpenCL library label Nov 17, 2025
Copy link
Contributor

Copilot AI left a 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 fixes a critical bug in the libclc atomic operation implementations where the increment and decrement functions were using the wrong underlying atomic operations. The __clc_atomic_inc function was incorrectly using __scoped_atomic_fetch_sub (subtraction) instead of __scoped_atomic_fetch_add (addition), and conversely, __clc_atomic_dec was using addition instead of subtraction.

  • Corrected __clc_atomic_inc to use __scoped_atomic_fetch_add instead of __scoped_atomic_fetch_sub
  • Corrected __clc_atomic_dec to use __scoped_atomic_fetch_sub instead of __scoped_atomic_fetch_add

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
libclc/clc/lib/generic/atomic/clc_atomic_inc.cl Fixed atomic increment to use addition operation instead of subtraction
libclc/clc/lib/generic/atomic/clc_atomic_dec.cl Fixed atomic decrement to use subtraction operation instead of addition

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

I think __clc_atomic_inc/dec should be deleted, or renamed. inc is a different op (i.e., atomicrmw uinc_wrap)

@wenju-he
Copy link
Contributor Author

I think __clc_atomic_inc/dec should be deleted, or renamed. inc is a different op (i.e., atomicrmw uinc_wrap)

can we add new __scoped_atomic_uinc_wrap/udec_wrap clang builtin and use it for __clc_atomic_inc/dec implementation?
our downstream implementation of __clc_atomic_inc does use uinc_wrap in atomicrmw.

@arsenm
Copy link
Contributor

arsenm commented Nov 19, 2025

I think __clc_atomic_inc/dec should be deleted, or renamed. inc is a different op (i.e., atomicrmw uinc_wrap)

can we add new __scoped_atomic_uinc_wrap/udec_wrap clang builtin and use it for __clc_atomic_inc/dec implementation? our downstream implementation of __clc_atomic_inc does use uinc_wrap in atomicrmw.

Yes, should do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libclc libclc OpenCL library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants