Clarification needed on valid types and specializations for atomic_ref

[PeterTh]

While working with SimSYCL we ran into a somewhat confusing aspect of the spec regarding atomic_ref.

In the beginning the spec rather clearly and unequivocally states the following:

The template parameter T must be one of the following types:

• int
• unsigned int
• long
• unsigned long
• long long
• unsigned long long
• float
• double

Notably absent are any pointer types.

However, at the same time, later on several functions available specifically on a partial specialization of atomic_ref<T*> are described.

This doesn't seem to make much sense to me. One way to read it would be to take it as T* for each of the Ts described previously being allowed, but if a pointer to some types is valid then there's no reason that another pointer to a different type should be invalid (that I can think of).

An easy way to solve it would be to add something like "any pointer type" to the list of allowed types at the start.

Off topic / only tangentially related: if this is touched, it might also be a good idea to use the opportunity to allow bool.

(See also celerity/SimSYCL#32)

[illuhad]

An easy way to solve it would be to add something like "any pointer type" to the list of allowed types at the start.

I think this is the intent. On a 64-bit system, you can always support T* for arbitrary T if you already support unsigned long long/uint64_t.

Off topic / only tangentially related: if this is touched, it might also be a good idea to use the opportunity to allow bool.

I think that not allowing bool is probably not just an oversight, and adding this would be a genuine new feature. bool typically maps to i8, and atomic support for 1-byte types is often spotty across backends/hardware. The same is true for 16-bit types. So I think that bool is missing for the same reason as short or half.

[Pennycook]

I think this is the intent. On a 64-bit system, you can always support T* for arbitrary T if you already support unsigned long long/uint64_t.

I agree that this is the intent. The specification even says in the description of fetch_add:

This function is only supported for 64-bit pointers on devices that have aspect::atomic64.

I think that adding "any pointer type" to the list of T is the right fix, and I agree with Aksel that we shouldn't add bool as a clarification. I would like us to eventually support arbitrary trivially copyable types, but the way to do that is probably via new features (e.g., new aspects for specific bit-widths as in sycl_ext_oneapi_atomic16, and/or a new aspect for lock-based atomics).

[gmlueck]

I think that adding "any pointer type" to the list of T is the right fix

I agree with this also, but we should consider whether we really mean "any" pointer type. For example, we do want to allow pointer to function, pointer to member, or pointer to member function types?

If we intend to exclude function, member, and member function pointers, we should say that T can be a pointer to object type:

The template parameter T must be one of the following types:

• [...]
• Any pointer to object type.

Or maybe:

The template parameter T must be one of the following types:

• [...]
• Any type U where U is a pointer to an object.

[Pennycook]

I agree with this also, but we should consider whether we really mean "any" pointer type. For example, we do want to allow pointer to function, pointer to member, or pointer to member function types?

Good point, I hadn't considered that. ISO C++ ([atomics.ref.pointer]) says that the specialization of std::atomic_ref is for "pointer-to-object types", so we should use your suggested "Any pointer-to-object type" wording.