Skip to content

[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

Closed
wants to merge 22 commits into from

Conversation

Alcpz
Copy link
Contributor

@Alcpz Alcpz commented Sep 22, 2023

The util header is divided in utility functions and math functions to help with machine translated code.

@Alcpz Alcpz requested a review from a team as a code owner September 22, 2023 10:22
@Alcpz Alcpz changed the title [SYCL][COMPAT] Headers updated to last version [SYCL][COMPAT] Headers updated to latest version Sep 22, 2023
@Alcpz Alcpz temporarily deployed to WindowsCILock September 22, 2023 10:23 — with GitHub Actions Inactive
@Alcpz Alcpz force-pushed the Alcpz/syclcompat-headers-update branch from 2112d80 to 3955940 Compare September 28, 2023 11:53
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 11:54 — with GitHub Actions Inactive
@Alcpz Alcpz changed the title [SYCL][COMPAT] Headers updated to latest version [SYCL][COMPAT] Util header updated to latest version Sep 28, 2023
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 12:16 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 12:32 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 12:38 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 13:00 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 13:56 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock September 28, 2023 14:19 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock October 6, 2023 13:25 — with GitHub Actions Inactive
@Alcpz Alcpz temporarily deployed to WindowsCILock October 6, 2023 13:47 — with GitHub Actions Inactive
// 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) {
Copy link
Contributor

@zhiweij1 zhiweij1 Dec 20, 2023

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.

Copy link
Contributor

@zhiweij1 zhiweij1 Dec 21, 2023

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);
}

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@Alcpz Alcpz force-pushed the Alcpz/syclcompat-headers-update branch from 79b7394 to acf930a Compare January 29, 2024 13:03
@Alcpz
Copy link
Contributor Author

Alcpz commented Mar 8, 2024

This PR grew too much in scope. In order to faciliate review, it is being splitted beginning with #12957

@Alcpz Alcpz closed this Mar 8, 2024
sommerlukas pushed a commit that referenced this pull request Mar 11, 2024
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
@Alcpz Alcpz deleted the Alcpz/syclcompat-headers-update branch June 27, 2024 15:46
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.

2 participants