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

[SYCL][DOC] Add spec and design for "if_device" #8917

Open
wants to merge 6 commits into
base: sycl
Choose a base branch
from

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Mar 31, 2023

Add a proposed extension specification and design for if_device, an alternative to __SYCL_DEVICE_ONLY__, which is implementable in a 1-pass compiler.

This commit also renames the proposed extension
"sycl_ext_oneapi_device_if" to "sycl_ext_oneapi_if_device_has" in order to avoid confusion with this new extension.

Add a proposed extension specification and design for `if_device`, an
alternative to `__SYCL_DEVICE_ONLY__`, which is implementable in a
1-pass compiler.

This commit also renames the proposed extension
"sycl_ext_oneapi_device_if" to "sycl_ext_oneapi_if_device_has" in order
to avoid confusion with this new extension.
@gmlueck gmlueck requested review from a team and tfzhu as code owners March 31, 2023 20:10
Allow the `fn` parameter to be a function pointer for the cases that
run on the host.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jun 30, 2023
@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 6, 2023

Ping @intel/dpcpp-doc-reviewers @tfzhu. We'd like to merge this if possible. We already have #10169 which implements this proposed extension for the current 2-pass compiler.

Pass the callable object using perfect forwarding, rather than by
value.  This enables (weird) cases where the callable object is not
copy constructible:

```
struct move_only {
  move_only();
  move_only(move_only &&other);
  void operator()() {/* code that runs only on device */}
};

void foo() {
  syclex::if_device(move_only{});
}
```
@gmlueck gmlueck requested a review from a team as a code owner July 28, 2023 19:06
@gmlueck gmlueck requested a review from steffenlarsen July 28, 2023 19:06

template<typename T>
static auto if_device(T &&fn) {
detail::call_if_on_device(fn);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rolandschulz

I think I do not need to use std::forward here, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Without the std::forward, if a r-value-ref gets passed to if_device then call_if_on_device gets passed a l-value-ref. If the callable only works with an r-value-ref this break. Always use forward for any universal reference (if you don't use it anymore afterwards).

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. Added in f3e15a7.

Use `std::forward` when passing callable object in code snippet.
Update the overview section and add an example showing how to
interpret data differently on host vs. device.
@gmlueck
Copy link
Contributor Author

gmlueck commented Sep 21, 2023

Ping to reviewers (@rolandschulz @steffenlarsen). If there are no more comments, I'd like to merge this PR. Merging is not a commitment to implement this feature. We can decide that separately.

@v-klochkov
Copy link
Contributor

This feature looks really useful, in particular we need in it in ESIMD component/development.
Unfortunately, it seems like this proposal is stuck. Is it blocked by some other ongoing SYCL changes/re-design activities?

@gmlueck
Copy link
Contributor Author

gmlueck commented Feb 22, 2024

This feature looks really useful, in particular we need in it in ESIMD component/development. Unfortunately, it seems like this proposal is stuck. Is it blocked by some other ongoing SYCL changes/re-design activities?

No, it's not blocked by anything in particular; our focus just turned to other things. I think we just need approvals from @intel/dpcpp-doc-reviewers and @intel/llvm-reviewers-runtime, and I need to fix the merge conflicts.

Copy link
Contributor

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Sep 16, 2024
@gmlueck gmlueck removed the Stale label Sep 20, 2024
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