[SYCL] Fix return type of relational functions on scalars #5975
Conversation
This fix updates scalar versions of relation functions that now return "bool" instead of a signed integer in SYCL 2020 mode.
The standard only references half support by redirecting to the OpenCL extensions and it's "int" there.
To the best of my reading of the standard previous implementation had a bug: SYCL 1.2.1, 4.13.1 Description of the built-in types available for SYCL host and device, table 4.109: genfloatd | double, doublen SYCL 1.2.1 4.13.7 Relational functions, table 4.116: igeninteger32bit isequal (genfloatf x, genfloatf y) igeninteger64bit isequal (genfloatd x, genfloatd y)
aelovikov-intel
changed the title
aelovikov-intel
changed the title
aelovikov-intel
changed the title
sycl/include/CL/sycl/builtins.hpp
Outdated
| template <typename T, | ||
| typename = detail::enable_if_t<detail::is_genfloat<T>::value, T>> | ||
| detail::common_rel_ret_t<T> signbit(T x) __NOEXC { | ||
| return detail::RelConverter<T>::apply( | ||
| __sycl_std::__invoke_SignBitSet<detail::rel_ret_t<T>>(x)); | ||
| } | ||
|
|
||
| // int any (sigeninteger x) | ||
| // The standard is being corrected for "any" at | ||
| // https://github.com/KhronosGroup/SYCL-Docs/pull/234. Scalar/marray version |
There was a problem hiding this comment.
That PR hasn't landed yet, so why it is OK to assume it will land and and in the way it is spelled currently?
@gmlueck : could you also confirm that this implementation change is valid now?
There was a problem hiding this comment.
We discussed this in the Khronos SYCL workgroup, and there was general agreement on the direction. I cannot say for sure that it will be accepted until it is merged, though. I do not mind if we put this PR on hold until the spec changes is accepted. There is a Khronos meeting today, but our next regular "issue" meeting is on April 14. There is some chance the spec PR will get merged today, but most likely it won't happen until April 14.
There was a problem hiding this comment.
Thanks @gmlueck .
@aelovikov-intel : please hold this PR until spec change lands.
There was a problem hiding this comment.
@aelovikov-intel : please hold this PR until spec change lands.
That's fine but I'd like to know if there are any issues with the code in advance. Can you both please do the actual review but hold on on approving it?
There was a problem hiding this comment.
I did review and it looks good to me.
There was a problem hiding this comment.
Note that the Khronos spec PR (KhronosGroup/SYCL-Docs#234) was accepted by the committee and merged.
* Additionally check for SYCL2020_CONFORMANT_APIS in SYCL2020 mode. * Don't fix SYCL 1.2.1 bug.
|
Ping @smaslov-intel . |
|
Would you please help to merge this in? I don't have write access. |
|
@aelovikov-intel, 1 LIT fail on windows in post-commit. Please follow up. |
)" This reverts commit fd2aa65.
This fix updates scalar versions of relation functions that now return "bool" instead of a signed integer in SYCL 2020 mode. The fix is ABI-breaking and is put under SYCL2020_CONFORMANT_APIS guard.
This fix updates scalar versions of relation functions that now return "bool" instead of a signed integer in SYCL 2020 mode. The fix is ABI-breaking and is put under SYCL2020_CONFORMANT_APIS guard. This is a revival of the reverted patch #5975. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com> Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>
The fix is ABI-breaking and is put under SYCL2020_CONFORMANT_APIS guard.