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.

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

Jump to bottom

Brick __brick_copy specialized by _ExecutionPolicy without std::decay_t #2112

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Brick __brick_copy specialized by _ExecutionPolicy without std::decay_t #2112

wants to merge 3 commits into from

Conversation

SergeyKopienko
Copy link
Contributor

In this PR we fix the issue #2110

@SergeyKopienko SergeyKopienko added this to the 2022.9.0 milestone Mar 6, 2025
@SergeyKopienko SergeyKopienko requested a review from mmichel11 March 6, 2025 17:09
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_brick_cpecializations branch from 66d050a to 91854a3 Compare March 6, 2025 17:10
@SergeyKopienko SergeyKopienko linked an issue Mar 6, 2025 that may be closed by this pull request
Brick __brick_copy specialized by _ExecutionPolicy without std::decay_t #2110
Open
@mmichel11
Copy link
Contributor

mmichel11 commented Mar 6, 2025 •
edited
Loading

How about just removing _ExecutionPolicy from the class templates of __brick_copy and __brick_copy_n? If we take a look at our specializations of __brick_copy, first on the host:

oneDPL/include/oneapi/dpl/pstl/algorithm_impl.h

Lines 1079 to 1105 in 417f118

template <class _Tag, typename _ExecutionPolicy>
struct __brick_copy<_Tag, _ExecutionPolicy, ::std::enable_if_t<__is_host_dispatch_tag_v<_Tag>>>
{
template <typename _RandomAccessIterator1, typename _RandomAccessIterator2>
_RandomAccessIterator2
operator()(_RandomAccessIterator1 __first, _RandomAccessIterator1 __last, _RandomAccessIterator2 __result,
/*vec*/ ::std::true_type) const
{
return __unseq_backend::__simd_assign(
__first, __last - __first, __result,
[](_RandomAccessIterator1 __first, _RandomAccessIterator2 __result) { *__result = *__first; });
}
template <typename _Iterator, typename _OutputIterator>
_OutputIterator
operator()(_Iterator __first, _Iterator __last, _OutputIterator __result, /*vec*/ ::std::false_type) const
{
return ::std::copy(__first, __last, __result);
}
template <typename _ReferenceType1, typename _ReferenceType2>
void
operator()(_ReferenceType1 __val, _ReferenceType2&& __result) const
{
__result = __val;
}
};
and secondly in the hetero backend:

oneDPL/include/oneapi/dpl/pstl/hetero/algorithm_impl_hetero.h

Lines 416 to 425 in 417f118

template <typename _BackendTag, typename _ExecutionPolicy>
struct __brick_copy<__hetero_tag<_BackendTag>, _ExecutionPolicy>
{
template <typename _SourceT, typename _TargetT>
void
operator()(_SourceT&& __source, _TargetT&& __target) const
{
__target = ::std::forward<_SourceT>(__source);
}
};
We see we use the tag to define these specializations whereas _ExecutionPolicy is unused.

Maybe I am misunderstanding something, but to me it looks like all we need is a tag.

@SergeyKopienko
Copy link
Contributor Author

I don't know the history why _ExecutionPolicy is used as brick's template parameter, so right now I don't think that their remove is correct. May be someone else explain these reasons.

@SergeyKopienko
include/oneapi/dpl/internal/async_impl/glue_async_impl.h - fix issue:…
9e64b3b 
… Brick __brick_copy specialized by _ExecutionPolicy without std::decay_t

#2110
@SergeyKopienko
include/oneapi/dpl/pstl/glue_algorithm_impl.h - fix issue: Brick __br…
578564d 
…ick_copy specialized by _ExecutionPolicy without std::decay_t

#2110
@SergeyKopienko
include/oneapi/dpl/pstl/glue_algorithm_ranges_impl.h - fix issue: Bri…
90baee6 
…ck __brick_copy specialized by _ExecutionPolicy without std::decay_t

#2110
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_brick_cpecializations branch from 91854a3 to 90baee6 Compare March 6, 2025 17:56
@danhoeflinger
Copy link
Contributor

I don't know the history why _ExecutionPolicy is used as brick's template parameter, so right now I don't think that their remove is correct. May be someone else explain these reasons.

My guess is that when we added tag dispatching, we replaced the needed functionality from the policy with functionality from the Tag, but neglected to remove the policy parameter then. I agree that we can just remove this template parameter.

@SergeyKopienko
Copy link
Contributor Author

I don't know the history why _ExecutionPolicy is used as brick's template parameter, so right now I don't think that their remove is correct. May be someone else explain these reasons.

My guess is that when we added tag dispatching, we replaced the needed functionality from the policy with functionality from the Tag, but neglected to remove the policy parameter then. I agree that we can just remove this template parameter.

Yes, @danhoeflinger, you are right:

  • as example I found the previous state of
template <typename _ExecutionPolicy>
struct __brick_copy<_ExecutionPolicy,
                   oneapi::dpl::__internal::__enable_if_hetero_execution_policy<_ExecutionPolicy, void>>
{
   // ...
}

@SergeyKopienko
Copy link
Contributor Author

I don't know the history why _ExecutionPolicy is used as brick's template parameter, so right now I don't think that their remove is correct. May be someone else explain these reasons.

My guess is that when we added tag dispatching, we replaced the needed functionality from the policy with functionality from the Tag, but neglected to remove the policy parameter then. I agree that we can just remove this template parameter.

@akukanov, @rarutyun could you please comment this moment?

@MikeDvorskiy
Copy link
Contributor

MikeDvorskiy commented Mar 7, 2025 •
edited
Loading 

oneapi::dpl::__internal::__enable_if_hetero_execution_policy<_ExecutionPolicy, void>>

Yes, before the tag dispatching policy type is used for type specialization..
But, now, when we have type specialization based on tag type trait, we don't need _ExecutionPolicy parameter.
Also, probably another bricks like oneapi::dpl::__internal::__brick_xxxx should be simplified as well.

@SergeyKopienko
Copy link
Contributor Author 

oneapi::dpl::__internal::__enable_if_hetero_execution_policy<_ExecutionPolicy, void>>

Yes, before the tag dispatching policy type is used for type specialization.. But, now, when we have type specialization based on tag type trait, we don't need _ExecutionPolicy parameter.

As far as I understood @rarutyun on our offline discussion with him, he has opposite opinion....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danhoeflinger danhoeflinger Awaiting requested review from danhoeflinger

@akukanov akukanov Awaiting requested review from akukanov

@MikeDvorskiy MikeDvorskiy Awaiting requested review from MikeDvorskiy

@mmichel11 mmichel11 Awaiting requested review from mmichel11

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
2022.9.0
Development

Successfully merging this pull request may close these issues.

Brick __brick_copy specialized by _ExecutionPolicy without std::decay_t
4 participants
@SergeyKopienko @mmichel11 @danhoeflinger @MikeDvorskiy