Skip to content

[SYCL] Add static cast for half builtins relational #6133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pvchupin merged 1 commit into from
May 10, 2022
Merged

[SYCL] Add static cast for half builtins relational #6133

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions sycl/source/detail/builtins_relational.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,42 +21,48 @@ __SYCL_INLINE_NAMESPACE(cl) {
namespace __host_std {
namespace {

template <typename T> inline T __vFOrdEqual(T x, T y) { return -(x == y); }
template <typename T> inline T __vFOrdEqual(T x, T y) {
Copy link
Contributor

@dkhaldi dkhaldi May 10, 2022

Choose a reason for hiding this comment

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

should these return a bool or T?
Where in the spec, this is stated?

Copy link
Contributor Author

@AidanBeltonS AidanBeltonS May 10, 2022

Choose a reason for hiding this comment

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

__vFOrdEqual is not stated in the spec. I am not sure as to if this should return a bool or T.
The goal of this change to resolve an issue with building DPCPP on windows and not to change any functionality.

Copy link
Contributor

@dkhaldi dkhaldi May 10, 2022

Choose a reason for hiding this comment

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

All these are part of 4.17.9. Relational functions. Some of them should return bool. However, you are right that the PR does not change any functionality. If there is a a problem, it was already there in the original code.

Copy link
Contributor

@aelovikov-intel aelovikov-intel May 10, 2022

Choose a reason for hiding this comment

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

I think functions modified here are all ok, because they're vector and vector versions don't return bool at all (even all/any).

As for the scalar version (not touched in this PR), this is slightly more complicated as SYCL1.2.1 and SYCL2020 differ here. Those, however, are all internal details and not the user-visible routines so technically we can have them defined whatever way we want to be able to implement user-visible wrappers in a way dependent on the presence of SYCL2020_CONFORMANT_APIS macro.

Copy link
Contributor

@v-klochkov v-klochkov May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

BTW, my guess why it returns -1 is because it is often good for vectorizer to have element filled with ones or with zeroes for using in vector mask operations like VANDPS/VORPS,
but I also was puzzled seeing return of -1.0 for float comparison.
It would be logically more correct to returning int32_t{-1} for float32 rather than -1.0

Copy link
Contributor

@aelovikov-intel aelovikov-intel May 10, 2022

Choose a reason for hiding this comment

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

User-visible methods must do exactly that according to the spec. I guess/think/hope that only those internal functions behave differently for some reason.

return -static_cast<T>(x == y);
}

template <typename T> inline T __sFOrdEqual(T x, T y) { return x == y; }

template <typename T> inline T __vFUnordNotEqual(T x, T y) { return -(x != y); }
template <typename T> inline T __vFUnordNotEqual(T x, T y) {
return -static_cast<T>(x != y);
}

template <typename T> inline T __sFUnordNotEqual(T x, T y) { return x != y; }

template <typename T> inline T __vFOrdGreaterThan(T x, T y) { return -(x > y); }
template <typename T> inline T __vFOrdGreaterThan(T x, T y) {
return -static_cast<T>(x > y);
}

template <typename T> inline T __sFOrdGreaterThan(T x, T y) { return x > y; }

template <typename T> inline T __vFOrdGreaterThanEqual(T x, T y) {
return -(x >= y);
return -static_cast<T>(x >= y);
}

template <typename T> inline T __sFOrdGreaterThanEqual(T x, T y) {
return x >= y;
}

template <typename T> inline T __vFOrdLessThanEqual(T x, T y) {
return -(x <= y);
return -static_cast<T>(x <= y);
}

template <typename T> inline T __sFOrdLessThanEqual(T x, T y) { return x <= y; }

template <typename T> inline T __vFOrdNotEqual(T x, T y) {
return -((x < y) || (x > y));
return -static_cast<T>((x < y) || (x > y));
}

template <typename T> inline T __sFOrdNotEqual(T x, T y) {
return ((x < y) || (x > y));
}

template <typename T> inline T __vLessOrGreater(T x, T y) {
return -((x < y) || (x > y));
return -static_cast<T>((x < y) || (x > y));
}

template <typename T> inline T __sLessOrGreater(T x, T y) {
Expand All @@ -67,7 +73,7 @@ template <typename T> s::cl_int inline __Any(T x) { return d::msbIsSet(x); }
template <typename T> s::cl_int inline __All(T x) { return d::msbIsSet(x); }

template <typename T> inline T __vOrdered(T x, T y) {
return -(
return -static_cast<T>(
!(std::isunordered(d::cast_if_host_half(x), d::cast_if_host_half(y))));
}

Expand Down Expand Up @@ -227,7 +233,7 @@ __SYCL_EXPORT s::cl_int FOrdLessThan(s::cl_half x, s::cl_half y) __NOEXC {
return (x < y);
}
__SYCL_EXPORT s::cl_short __vFOrdLessThan(s::cl_half x, s::cl_half y) __NOEXC {
return -(x < y);
return -static_cast<s::cl_short>(x < y);
}
MAKE_1V_2V_FUNC(FOrdLessThan, __vFOrdLessThan, s::cl_int, s::cl_float,
s::cl_float)
Expand Down