[SYCL] Specialize atomic fetch_add for floating point types

[AGindinson]

The new EXT/SPV_EXT_shader_atomic_float_add SPIR-V extension
allows us to further specialize atomic::fetch_add() for
floating point types. In device mode, we'll now be creating
an external call to a built-in-like __spirv_AtomicFAddEXT().
This is similar to what is done for other atomic binary
instructions, e.g. the integer specialization of fetch_add()
being mapped onto __spirv_AtomicIAdd().

Furthermore, atomic::fetch_sub() is also re-implemented
to use __spirv_AtomicFAddEXT(), the added operand being
a negation of the original one.

The new implementation can be exposed if a dedicated macro is
defined: SYCL_USE_NATIVE_FP_ATOMICS. Otherwise, a fallback
is used, where the atomic operation is done via spinlock emulation.
At the moment of committing this, only Intel GPUs support the
"native" implementation, which relies on a SPIR-V extension.

Tests for the feature have been finalized in
intel/llvm-test-suite#104.

Signed-off-by: Artem Gindinson artem.gindinson@intel.com

[Pennycook]

The changes look good to me.

One quick question to make sure I understand: am I right in thinking that we now default to the native FAdd implementation for all our devices? So the old compare-exchange implementation will now only be tested for CUDA devices?

@bader We should add a TODO somewhere to implement AtomicFAddEXT in the CUDA backend.

[bader]

We should add a TODO somewhere to implement AtomicFAddEXT in the CUDA backend.

I suggest adding a new issue with "cuda" tag - https://github.com/intel/llvm/issues.

[AGindinson]

One quick question to make sure I understand: am I right in thinking that we now default to the native FAdd implementation for all our devices? So the old compare-exchange implementation will now only be tested for CUDA devices?

Yes, this is aimed for all device BEs. I'll create an issue for CUDA once the extension support is pulled down into this repository.

[AGindinson]

@Pennycook, could you please check if __SYCL_EMULATE_NATIVE_ATOMICS__ is a fine name for the macro (as introduced by d8712b5)?

Also, should the IR checks in test/atomic_ref/ be updated to include the emulated checks under the macro defined, or would it be sufficient to update github.com/intel/llvm-test-suite to use the macro for non-supporting targets and make sure all tests pass?

[AGindinson]

/summary:run

[Pennycook]

@Pennycook, could you please check if __SYCL_EMULATE_NATIVE_ATOMICS__ is a fine name for the macro (as introduced by d8712b5)?

That commit says __SYCL_EMULATE_FLOAT_ATOMICS__ rather than __SYCL_EMULATE_NATIVE_ATOMICS__. But I think __SYCL_EMULATE_FLOAT_ATOMICS__ is a good name.

Also, should the IR checks in test/atomic_ref/ be updated to include the emulated checks under the macro defined, or would it be sufficient to update github.com/intel/llvm-test-suite to use the macro for non-supporting targets and make sure all tests pass?

Good question. If we're documenting __SYCL_EMULATE_FLOAT_ATOMICS__ then I think we need to update the tests -- a device that implements AtomicFAdd would need to run correctly with and without the macro enabled, and so we should test that to ensure we don't accidentally break things. If we're not documenting __SYCL_EMULATE_FLOAT_ATOMICS__, and it's only a temporary implementation detail, I think it's sufficient to ensure that the existing tests pass when the macro is set to the expected value.

[AGindinson]

That commit says SYCL_EMULATE_FLOAT_ATOMICS rather than SYCL_EMULATE_NATIVE_ATOMICS. But I think SYCL_EMULATE_FLOAT_ATOMICS is a good name.

That was an unfortunate typo on my side, sorry for that.

A device that implements AtomicFAdd would need to run correctly with and without the macro enabled.

Fair enough. And yet I tend to agree that:

it's only a temporary implementation detail

My personal approach would be to rely on the E2E tests passing with correct macro presence/absence. Should there be a lack of consensus among the reviewers, though, I'm prepared to lay this back and make all testing stages as detailed as possible. How would you recommend treating this? @AlexeySotkin, @AlexeySachkov, could you comment as well, please?

[AGindinson]

Updates:

1. Switched to using the emulated implementation as default.
2. Renamed the macro correspondingly - it's SYCL_USE_NATIVE_FP_ATOMICS from now on. This is reflected in the test-suite PR: Address the atomics' macro rework: emulation runs by default llvm-test-suite#104
3. Removed the changes from atomic.hpp, as this would've regressed the common atomic class for other targets. atomic_ref remains as the go-to class for native atomics.

[romanovvlad]

LGTM