-
Notifications
You must be signed in to change notification settings - Fork 789
[SYCL][COMPAT] Util header updated to latest version #11267
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.
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
Conversation
2112d80
to
3955940
Compare
sycl/include/syclcompat/math.hpp
Outdated
// For floating-point types, `float` or `double` arguments are acceptable. | ||
// For integer types, `std::uint32_t`, `std::int32_t`, `std::uint64_t` or | ||
// `std::int64_t` type arguments are acceptable. | ||
inline double min(const double a, const float b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Alcpz , we recently found an issue in these min/max overloads.
In this implementation, we use std::int??_t/uint??_t, but these types are not fundamental types.
If user pass 2 longlong type variables to dpct::max, and if int64_t is typedef of long, then compiler does not know which overload should be chosen.
And when we want to add a new longlong overload, it is difficult to know if there is redefinition since which type is chosen to define int64_t is implementation defined. (gcc treats int64_t and long are same, but msvc treats int64_t and longlong are same).
So I suggest using fundamental types (int/long/longlong/...) here instead of fixed width integer types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is something like below better?
template <typename T1, typename T2>
std::enable_if_t<std::is_integral_v<T1> && std::is_integral_v<T2>,
std::common_type_t<T1, T2>>
min(T1 a, T2 b) {
return sycl::min<std::common_type_t<T1, T2>>(a, b);
}
template <typename T1, typename T2>
std::enable_if_t<std::is_floating_point_v<T1> && std::is_floating_point_v<T2>,
std::common_type_t<T1, T2>>
min(T1 a, T2 b) {
return sycl::fmin<std::common_type_t<T1, T2>>(a, b);
}
template <typename T1, typename T2>
std::enable_if_t<std::is_integral_v<T1> && std::is_integral_v<T2>,
std::common_type_t<T1, T2>>
max(T1 a, T2 b) {
return sycl::max<std::common_type_t<T1, T2>>(a, b);
}
template <typename T1, typename T2>
std::enable_if_t<std::is_floating_point_v<T1> && std::is_floating_point_v<T2>,
std::common_type_t<T1, T2>>
max(T1 a, T2 b) {
return sycl::fmax<std::common_type_t<T1, T2>>(a, b);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this @zhiweij1. I will add the changes you propose once I find time to get back to these PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR is closed, I will ping you in the appropriate PR that contains these changes
79b7394
to
acf930a
Compare
…ed from SYCLCompat
…d values. fix: Added default assertion in case a non contemplated datatype uses the OpTestLauncher classes to avoid silent failures.
Rebased test_isnan to math_basic.
Fix: Checking return type instead of argument types for result checks in tests.
…loating point datatypes
… the documented behavior.
0862b3d
to
8a72a75
Compare
This PR grew too much in scope. In order to faciliate review, it is being splitted beginning with #12957 |
This PR prepares SYCLcompat to receive multiple PRs containing updates to the helper headers. It will substitute the previous opened PR as it grew in scope too much, as in case of issues it would be difficult to track and resolve effectively the problem. Edit: Previous PR was #11267
The util header is divided in utility functions and math functions to help with machine translated code.