Skip to content
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

Fix compile error in __serial_merge #2013

Merged
merged 11 commits into from
Jan 22, 2025

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented Jan 22, 2025

In this PR we fix compile errors in __serial_merge function for case when the types of __rng2 and __rng1 aren't the same:

/include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_merge.h:162:17: error: conditional expression is ambiguous; '__return_t' (aka 'const oneapi::dpl::__internal::tuple<unsigned long, unsigned long>') can be converted to 'decltype(this->make_reference(::std::declval<_tuple_ranges_t>(), __i, ::std::make_index_sequence<__num_ranges>()))' (aka 'tuple<const unsigned long &, const unsigned long &>') and vice versa
  162 |                 ? __rng2[__rng2_idx++]
      |                 ^ ~~~~~~~~~~~~~~~~~~~~
  163 |                 : __rng1[__rng1_idx++];
      |                   ~~~~~~~~~~~~~~~~~~~~
/include/oneapi/dpl/pstl/hetero/dpcpp/parallel_backend_sycl_merge.h:198:21: note: in instantiation of function template specialization 'oneapi::dpl::__par_backend_hetero::__serial_merge<oneapi::dpl::__ranges::zip_view<oneapi::dpl::__ranges::all_view<unsigned long, sycl::access::mode::read>, oneapi::dpl::__ranges::all_view<unsigned long, sycl::access::mode::read>>, oneapi::dpl::__ranges::all_view<oneapi::dpl::__internal::tuple<unsigned long, unsigned long>, sycl::access::mode::read>, const oneapi::dpl::__ranges::zip_view<oneapi::dpl::__ranges::all_view<unsigned long, sycl::access::mode::write>, oneapi::dpl::__ranges::all_view<unsigned long, sycl::access::mode::write>>, unsigned int, dpct::internal::compare_key_fun<>>' requested here
  198 |                     __serial_merge(__rng1, __rng2, __rng3, __start.first, __start.second, __i_elem, __chunk, __n1, __n2,
      |                     ^
/include/oneapi/dpl/pstl/hetero/algorithm_impl_hetero.h:1821:37: note: in instantiation of function template specialization 'oneapi::dpl::__internal::__pattern_merge<oneapi::dpl::__internal::__device_backend_tag, oneapi::dpl::execution::device_policy<oneapi::dpl::__internal::__set_union_copy_case_2<SetUnionByKey>>, oneapi::dpl::zip_iterator<oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, oneapi::dpl::__internal::tuple<unsigned long, unsigned long>>, oneapi::dpl::zip_iterator<oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>>, dpct::internal::compare_key_fun<>>' requested here
 1821 |     return oneapi::dpl::__internal::__pattern_merge(
      |                                     ^
/include/oneapi/dpl/pstl/glue_algorithm_impl.h:1007:37: note: in instantiation of function template specialization 'oneapi::dpl::__internal::__pattern_set_union<oneapi::dpl::__internal::__device_backend_tag, oneapi::dpl::execution::device_policy<SetUnionByKey> &, oneapi::dpl::zip_iterator<oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>>, oneapi::dpl::zip_iterator<oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>>, oneapi::dpl::zip_iterator<oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>>, dpct::internal::compare_key_fun<>>' requested here
 1007 |     return oneapi::dpl::__internal::__pattern_set_union(__dispatch_tag, ::std::forward<_ExecutionPolicy>(__exec),                                                                |                                     ^
/include/dpct/dpl_extras/algorithm.h:840:23: note: in instantiation of function template specialization 'oneapi::dpl::set_union<oneapi::dpl::execution::device_policy<SetUnionByKey> &, oneapi::dpl::zip_iterator<oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>>, oneapi::dpl::zip_iterator<oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>>, oneapi::dpl::zip_iterator<oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>, oneapi::dpl::__internal::sycl_iterator<sycl::access::mode::read_write, unsigned long>>, dpct::internal::compare_key_fun<>>' requested here
  840 |   auto ret_val = std::set_union(                                                               

This compile error has been introduced in the PR #1970

…fix compile error in __serial_merge for case when the types of __rng2 and __rng1 aren't the same
…fix compile error in __serial_merge for case when the types of __rng2 and __rng1 aren't the same
…fix compile error in __serial_merge for case when the types of __rng2 and __rng1 aren't the same
…remove local variables from the for-loop in __serial_merge
…remove local variables from the for-loop in __serial_merge
…fix compile error in __serial_merge for case when the types of __rng2 and __rng1 aren't the same
…fix compile error in __serial_merge for case when the types of __rng2 and __rng1 aren't the same
…fix compile error in __serial_merge for case when the types of __rng2 and __rng1 aren't the same
…fix review comment: remove ternary operator at all
…revert: remove local variables from the for-loop in __serial_merge
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

Fix looks good with Green CI.

Does testing cover this case or should we add a test to prevent regression?

@SergeyKopienko
Copy link
Contributor Author

SergeyKopienko commented Jan 22, 2025

Does testing cover this case or should we add a test to prevent regression?

It's not covered by our tests.
And it's not simple different types of source data for merge, these two types may be converted from one another and back versa:

But may this mean that we has some problem in the implementation of oneapi::dpl::__internal::tuple ?

I mean, it implements more then one conversion operators :

    // conversion to ::std::tuple with the same template arguments
    operator ::std::tuple<T1, T...>() const
    {
        static constexpr ::std::size_t __tuple_size = sizeof...(T) + 1;
        return to_std_tuple(*this, ::std::make_index_sequence<__tuple_size>());
    }

    // conversion to ::std::tuple with the different template arguments
    template <typename U1, typename... U>
    operator ::std::tuple<U1, U...>() const
    {
        constexpr ::std::size_t __tuple_size = sizeof...(T) + 1;
        return to_std_tuple(static_cast<tuple<U1, U...>>(*this), ::std::make_index_sequence<__tuple_size>());
    }

And it's looks like the root case of this compile error.
Sure, this PR will fix them. But is it enought?

@danhoeflinger
Copy link
Contributor

But may this mean that we has some problem in the implementation of oneapi::dpl::__internal::tuple ?

Ah, interesting, you may be correct...
We would have to be very careful with any changes here.

I need to investigate about what implicit conversion is allowed between std::tuple<T...> and a differently but per-element convertible typed std::tuple<U...>. I wonder if we should only have one of these implicit conversions. The second option maybe should cover the first.

@SergeyKopienko
Copy link
Contributor Author

I created new issue for the future investigations: #2017

danhoeflinger
danhoeflinger previously approved these changes Jan 22, 2025
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

The fix LGTM.

…fix review comment: remove redundant parentheses
Copy link
Contributor

@MikeDvorskiy MikeDvorskiy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

Still LGTM after removal of parens.

@timmiesmith timmiesmith merged commit b619105 into main Jan 22, 2025
22 checks passed
@timmiesmith timmiesmith deleted the dev/skopienko/fix_compile_error_in_serial_merge branch January 22, 2025 20:19
SergeyKopienko added a commit that referenced this pull request Jan 27, 2025
SergeyKopienko added a commit that referenced this pull request Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants