[SYCL] float atomic_ref performance fix

[cperkinsintel]

Moving the load into the CAS loop greatly improves performance, especially on GPU. It isn't entirely clear to me why this should produce such a dramatic improvement.

Signed-off-by: Chris Perkins chris.perkins@intel.com

[Pennycook]

Do you see the same improvement just from moving the load inside of the loop, or do you need the reinterpret_cast changes as well? I'm pretty sure that the reinterpret_cast usage here is undefined behavior, and we shouldn't rely on it.

Tagging @rolandschulz to confirm.

[cperkinsintel]

@Pennycook - The most significant performance improvement definitely comes from the .load change.
On Tuesday I will run some tests to get a better picture of how much the memcpy is costing us, so we can decide whether to keep them or not.

I am not sure I understand why the reinterpret cast would be UB in this case. I certainly know it's not a good practice generally and can result in UB if you aren't careful, but we have static_asserts guaranteeing the two types are the same size. We are forcibly moving between floats and int (and vice versa). Before this PR, bit_cast was calling detail::memcpy which loops through the individual bytes as if they are chars. To me that does not seem preferable from any standpoint, especially performance.

[Pennycook]

On Tuesday I will run some tests to get a better picture of how much the memcpy is costing us, so we can decide whether to keep them or not.

Great, thanks!

I am not sure I understand why the reinterpret cast would be UB in this case. I certainly know it's not a good practice generally and can result in UB if you aren't careful, but we have static_asserts guaranteeing the two types are the same size. We are forcibly moving between floats and int (and vice versa).

The pointer that you get back from reinterpret_cast is still bound by C++ strict aliasing rules. The compiler is free to assume that the float* and int* point to different memory because they are different types. reinterpret_cast is safe for conversions to char* because of aliasing rules, but isn't safe for type-punning in general. That's why C++20 gives us bit_cast.

Before this PR, bit_cast was calling detail::memcpy which loops through the individual bytes as if they are chars. To me that does not seem preferable from any standpoint, especially performance.

The memcpy in atomic.hpp should be replaced by a bit_cast. But the one in bit_cast itself is there as a fallback, and should only be called if the compiler doesn't have native support for bit_cast. Are you seeing memcpy generated? If so, that sounds like a bug.

Even when the memcpy is there, the intent is that the compiler recognizes it as type-punning and optimizes it away.

[Pennycook]

I'm still concerned that we don't understand why this helps, but since the changes I requested have been implemented I'll mark this as approved.

[againull]

/summary:run

[cperkinsintel]

pinging the runtime owners. In a separate discussion it appears everyone is ok with this change. s.b. ready to merge.