[SYCL][HIP] Change atomic sub to atomic add (-val)

[hdelan]

This PR changes the use of atomic_sub to atomic_add with a negated val. Doing so extends the number of cases in which the atomic operation will work, since support for atomic_add is more common than atomic_sub, especially at the PCIe level.

[hdelan]

Related to #11218 as well as #7252

[jchlanda]

making this change allows atomic_ref::fetch_sub to work in a wider variety of cases

Does that mean that there is still a subset of devices for which the bug exists.

Also, if I remember it right, it's the first commit message (not the PR info) that gets added to intel/llvm, so I would reword it to be more like your PR's title and the explanation for the change.

[hdelan]

First comment again:

When using HIP atomics with malloc_shared allocation, ROCm may use PCIe level atomics in order to optimize operations. More information can be found here https://rocm.docs.amd.com/en/latest/understand/More-about-how-ROCm-uses-PCIe-Atomics.html .

PCIe support for atomic_sub is not as common as PCIe support for atomic_add, so making this change allows atomic_ref::fetch_sub to work in a wider variety of cases, most notably when using atomics on part of a malloc_shared allocation

[hdelan]

Does that mean that there is still a subset of devices for which the bug exists.

@jchlanda Atomic add is guaranteed to work for PCIe 3. And that dates from 2010 I think. See here https://www.intel.sg/content/dam/doc/white-paper/pci-express3-accelerator-white-paper.pdf

[hdelan]

Ping @intel/llvm-gatekeepers

[aelovikov-intel]

Testing is old and the change is AMD GPU specific. We seem to have it broken currently, and I'm not sure if anybody is looking into it. I wouldn't want to merge until CI is fixed.

[jinz2014]

@hdelan I have a question. Is double-precision floating-point subtract implemented elsewhere ?

[hdelan]

Hi @jinz2014 that's a good question. __spirv_AtomicFAddEXT is an extension for floating point add in SPIRV, which we need to implement in libclc for fp32 and fp64. However __spirv_AtomicFSubEXT is actually not a SPIR-V extension and is not used in the SYCL headers. So the symbol was unused and floating. So I will remove it now. Good catch, thanks

[jinz2014]

No problem. I didn't catch it. So fp32 and fp64 are implemented in another file in libclc.

[hdelan]

No problem. I didn't catch it. So fp32 and fp64 are implemented in another file in libclc.

Well they aren't actually implemented at all for floating point. If floating point sub is needed then you need to rely on fetch_add(-val); in SYCL, since SYCL only uses atomic fAdd not atomic fSub

[hdelan]

@aelovikov-intel it looks like CI is working again? Can this be merged?

[aelovikov-intel]

Quick question first. Any chance a-b and a+(-b) behave differently under IEEE-754? Like some weird rounding/signed zeroes/nans/etc.?

[hdelan]

So it's only different for unsigned types where a + (-b) will cause overflow for -b. But this is OK because unsigned overflow is not UB so this is guaranteed to always work. Note that HIP uses + (-val) for its atomics as well, presumably due to the better PCIe support for atomic add https://github.com/ROCm-Developer-Tools/clr/blob/develop/hipamd/include/hip/amd_detail/amd_hip_atomic.h#L305

[jinz2014]

The link gives me some light of the HIP implementation. Thanks.