Skip to content

GPU: make the Metal atomics operate on plain counters - #15944

Merged
ktf merged 2 commits into
AliceO2Group:devfrom
ktf:pr15944
Oct 14, 2026
Merged

ktf merged 2 commits into
AliceO2Group:devfrom
ktf:pr15944

Conversation

@ktf

@ktf ktf commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

GPUAtomic() expanded to metal::atomic, but the counters it guards are plain
integers in structures that the host allocates and transfers, and the code reads
them directly outside the atomic operations. That is exactly the CUDA and HIP
situation, where GPUAtomic() is a no-op and atomicAdd() takes a plain pointer,
so Metal now does the same.

MSL's atomic_*_explicit do want metal::atomic, which has the same size and
alignment, so the operand is cast at the one point where the operation is
issued. That cast has to be made in the address space the pointer is really in,
since casting a threadgroup pointer to a device atomic is a hard error, hence
the two overloads: threadgroup stays threadgroup for the *Shared entry points,
anything else resolves to device.

@ktf
ktf requested a review from davidrohr as a code owner October 12, 2026 13:24

@davidrohr davidrohr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The atomic part inside the #idef METAL is of course fine.
You added GPUsharedref in the clusterizer. I assume this was only needed for OpenCL1, which we don't use anymore, so in that sense you are lucky, that we still have the GPUsharedref in most places. But you might need to fix it up in more places, let's see. Only point is: I would separate it into 2 commits, for atomics and sharedref.

@ktf

ktf commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Notice that the shared ref is actually a requirement for the atomics to work. It's there to avoid that some threadgroup pointer is silently then compiled against a device one.

With the threadgroup qualifier visible, casting to a device atomic is a hard error ("converts between mismatching address spaces"), but a generic pointer both misses the threadgroup overload and launders that cast past the compiler, so the counter would get a device atomic with no diagnostic.

@davidrohr

Copy link
Copy Markdown
Collaborator

Yes, my point was that it is still 2 fixes: 1. you need to add the sharedref, since it is simply missing, and the other backends do not care since they do not need it (after OpenCL1 was removed).
And then the METAL atomic change is a totally different topic IMHO.

@ktf

ktf commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

ok, I will split it.

ktf added 2 commits October 14, 2026 13:36
Will be needed by a subsequent fix for the atomics.
GPUAtomic() expanded to metal::atomic, but the counters it guards are plain
integers in structures that the host allocates and transfers, and the code reads
them directly outside the atomic operations. That is exactly the CUDA and HIP
situation, where GPUAtomic() is a no-op and atomicAdd() takes a plain pointer,
so Metal now does the same.

MSL's atomic_*_explicit do want metal::atomic, which has the same size and
alignment, so the operand is cast at the one point where the operation is
issued. That cast has to be made in the address space the pointer is really in,
since casting a threadgroup pointer to a device atomic is a hard error, hence
the two overloads: threadgroup stays threadgroup for the *Shared entry points,
anything else resolves to device.
@ktf

ktf commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Ok, splitted.

@ktf

ktf commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Split done, fullCI works. All changes are no-op when running on linux. Merging.

@ktf
ktf merged commit f47d9b5 into AliceO2Group:dev Oct 14, 2026
12 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants