Skip to content

Some bricks specialized by ExecutionPolicy without apply std::decay_t #2129

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 30 commits into from

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented Mar 13, 2025

In this PR we fix the issue #2110

As we discussed with @rarutyun we still need to have EcecutionPolicy in bricks from architecture point of view.

This PR is a new copy of #2112

Implementation details

Now in all struct __brick_... implementations we have new static_assert :

static_assert(std::is_same_v<_ExecutionPolicy, std::decay_t<_ExecutionPolicy>>);

So we will have compile error if we use brick with non-decayed policy.

@SergeyKopienko SergeyKopienko added this to the 2022.9.0 milestone Mar 13, 2025
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_brick_specializations branch from d5e35c4 to 7dc3b40 Compare March 13, 2025 14:07
@SergeyKopienko SergeyKopienko changed the title Some bricks specialized by ExecutionPolicy without std::decay_t call Some bricks specialized by ExecutionPolicy without apply std::decay_t Mar 13, 2025
@SergeyKopienko
Copy link
Contributor Author

@akukanov, @rarutyun could you please take a look to this PR ?

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.

My first reaction to this is that it is very likely a good change.
However, I think we need to wait to consider this in the context of a larger discussion about differing reference types of arguments (Policy and ranges) and SYCL kernels, which I hope will happen in the next couple weeks.

We first need to decide what the expected behavior is when a user passes different flavored reference types of policies and ranges, and if that changes for different contexts:

  1. no unnamed lambda vs unnamed lambda
  2. same kernel name specified vs different kernel name specified vs none specified

Once we have some conclusions about expected behavior there, we can figure out the best steps to achieve that.

I believe something like this will be part of the solution. However, I would resist merging any related changes until we can take a step back and consider it at from a higher level.

@danhoeflinger
Copy link
Contributor

danhoeflinger commented Mar 13, 2025

(If a user is waiting on a specific fix for a build error, that may change the calculus here, but I don't think that is the case)

Comment on lines 82 to 87
using _DecayedExecutionPolicy = std::decay_t<_ExecutionPolicy>;

wait_for_all(::std::forward<_Events>(__dependencies)...);
auto ret_val = oneapi::dpl::__internal::__pattern_walk2_brick_async(
__dispatch_tag, ::std::forward<_ExecutionPolicy>(__exec), __first, __last, __result,
oneapi::dpl::__internal::__brick_copy<decltype(__dispatch_tag), _ExecutionPolicy>{});
oneapi::dpl::__internal::__brick_copy<decltype(__dispatch_tag), _DecayedExecutionPolicy>{});
Copy link
Contributor

@akukanov akukanov Mar 14, 2025

Choose a reason for hiding this comment

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

Here comes a subjective cosmetic nitpicking :)

In this and many other functions, _DecayedExecutionPolicy is only used once in its definition scope. Meanwhile, it saves just 7 symbols, comparing to std::decay_t<_ExecutionPolicy>, and adds nothing to readability.

I see little value in the alias even where it is reused, and absolutely zero (that is, -273 :)) reasons to add it when used only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am ready to any variant, it's not problem....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@SergeyKopienko SergeyKopienko requested a review from akukanov March 17, 2025 08:40
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_brick_specializations branch 2 times, most recently from bfdc196 to ab9cbaa Compare March 19, 2025 13:13
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_brick_specializations branch from ab9cbaa to 3e57cbb Compare March 26, 2025 17:03
@SergeyKopienko SergeyKopienko deleted the dev/skopienko/fix_brick_specializations branch March 26, 2025 17:45
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.

Brick __brick_copy specialized by _ExecutionPolicy without std::decay_t
3 participants