Skip to content

Conversation

@PietroGhg
Copy link
Contributor

This PR sets SYCL_USE_NATIVE_FP_ATOMICS when compiling for Native CPU, and provides an implementation for said atomics via an LLVM pass that defines them through atomicrmw instructions.

Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

What about fetch_sub? I can see the following in SYCL spec for atomic_ref:

Floating fetch_sub(Floating operand,
                     memory_order order = default_read_modify_write_order,
                     memory_scope scope = default_scope) const noexcept;

@PietroGhg
Copy link
Contributor Author

What about fetch_sub? I can see the following in SYCL spec for atomic_ref:

Floating fetch_sub(Floating operand,
                     memory_order order = default_read_modify_write_order,
                     memory_scope scope = default_scope) const noexcept;

Thanks @maarquitos14 , it appears to be implemented as atomicfadd(-operand) see atomic_ref.hpp

Copy link
Contributor

@maarquitos14 maarquitos14 left a comment

Choose a reason for hiding this comment

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

LGTM.

@maarquitos14
Copy link
Contributor

Would it be possible to add e2e test to also check that we get the expected result?

if (DeviceTriple.isNVPTX() || DeviceTriple.isAMDGPU() ||
(DeviceTriple.isSPIR() &&
DeviceSubArch != llvm::Triple::SPIRSubArch_fpga))
DeviceSubArch != llvm::Triple::SPIRSubArch_fpga) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a FE test? I assume there is already a FE test checking the existence of this macro for other targets. You can just add a RUN there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it, thank you

@PietroGhg
Copy link
Contributor Author

Would it be possible to add e2e test to also check that we get the expected result?

There are already e2e tests that cover fp atomics, currently we track the pass rate of those internally for Native CPU

Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

FE LGTM. Thanks!

@PietroGhg
Copy link
Contributor Author

@intel/llvm-gatekeepers, this looks ready to merge, thank you

@martygrant martygrant merged commit 6dd4984 into intel:sycl Nov 4, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants