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] Adopt the experimental free function extension #2073

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Aug 10, 2023

Access the current nd_item via a free function instead of storing it as a data member of the various SYCL accelerator base classes.

Copy link
Member

@bernhardmgruber bernhardmgruber left a comment

Choose a reason for hiding this comment

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

LGTM. Here is a further suggestion:

include/alpaka/acc/AccGenericSycl.hpp Outdated Show resolved Hide resolved
Access the current nd_item via a free function instead of storing it
as a data member of the various SYCL accelerator base classes.
@fwyzard fwyzard force-pushed the sycl_experimental_free_functions branch from 5ea8f82 to 39d86c6 Compare August 10, 2023 11:41
@fwyzard
Copy link
Contributor Author

fwyzard commented Aug 10, 2023

I will keep this as a draft while we check the impact on the performance.

@AuroraPerego
Copy link
Contributor

AuroraPerego commented Aug 11, 2023

The performance does not change:

nThreads base@c9dbac0 base + PR
1 148.44 +/- 3.48) 148.32 +/- 0.35)
2 172.73 +/- 0.94) 171.72 +/- 0.60)
4 181.79 +/- 0.16) 182.07 +/- 0.19)
8 182.89 +/- 0.06) 182.94 +/- 0.15)

However, since this feature is still experimental and there is another one similar to it as proposed, maybe it's better to wait a bit and see if the extension gets modified.

The tests done with pixeltrack-standalone@dcf2898.
The numbers are the throughput, i.e. the number of concurrent events processed per second, computed processing 10000 events and nThreads is the number of CPU threads used.

Copy link
Member

@j-stephan j-stephan left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to convert this into a real PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants