Skip to content

[SYCL] Decouple data types of SYCL builtins from OpenCL. #669

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

Merged

Conversation

alexeyvoronov-intel
Copy link
Contributor

  • Removed all use of OpenCL types from SYCL header files, aside from converting into SPIRV type.
  • Removed some code dublications.
  • Aligned built-in's host version with SPIRV types.
  • Aligned nan built-in implementation with SYCL specification.
  • Renamed detail/type_tratis.hpp to detail/std_type_tratis.hpp
  • Updated SFINAE logic of intel/sub_group.hpp
  • Added test for built-in's type_tratis

Signed-off-by: Alexey Voronov [email protected]

@sergey-semenov sergey-semenov self-assigned this Sep 24, 2019
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

LGTM, some minor comments.

@alexeyvoronov-intel alexeyvoronov-intel force-pushed the private/avoronov/gen_type_traits_ref branch from 474b396 to cb1f51d Compare September 26, 2019 16:39
return __sycl_std::__invoke_half_sqrt<T>(x);
}

// genfloatf tan (genfloatf x)
template <typename T>
typename std::enable_if<detail::is_genfloatf<T>::value, T>::type
tan(T x) __NOEXC {
detail::enable_if_t<detail::is_genfloatf<T>::value, T> tan(T x) __NOEXC {
Copy link
Contributor

Choose a reason for hiding this comment

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

Arrived here I have an overdose of detail::enable_if_t<detail::is_genfloatf<T>::value, T> :-)
What about defining a meta-function with using to factorize out this?
Add as many meta-function you can to reduce the code size...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I agree with your comment, but I'd prefer not to inflate this patch any further.
In the near future I will create a couple of patches for this functionality and I will apply your suggestion in them.
Could you provide a small code snippet of what you would like this code to look like?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to find a name understandable by native English speakers, which I am not...
A few ideas

detail::enable_if_genfloat_t<T> tan(T x) __NOEXC {
detail::only_genfloat_t<T> tan(T x) __NOEXC {
detail::only<detail::is_genfloatf, T> tan(T x) __NOEXC {

@alexeyvoronov-intel alexeyvoronov-intel force-pushed the private/avoronov/gen_type_traits_ref branch from cb1f51d to 5f1d64b Compare September 27, 2019 12:12
sergey-semenov
sergey-semenov previously approved these changes Sep 27, 2019
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

LGTM, approved unless @keryell has any further comments

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

A few minor comments.

Removed all use of OpenCL types from SYCL header files,
  aside from converting into SPIRV type.
Removed some code dublications.
Aligned built-in's host version with SPIRV types.
Aligned nan built-in implementation with SYCL specification.
Renamed detail/type_tratis.hpp to detail/std_type_tratis.hpp
Updated SFINAE logic of intel/sub_group.hpp
Added test for built-in's type_tratis

Signed-off-by: Alexey Voronov <[email protected]>
@bader bader merged commit 6bcf548 into intel:sycl Sep 28, 2019
@alexeyvoronov-intel alexeyvoronov-intel deleted the private/avoronov/gen_type_traits_ref branch November 11, 2019 16:27
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.

4 participants