-
Notifications
You must be signed in to change notification settings - Fork 738
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] Implement max_num_work_groups from the launch queries extension #14333
Changes from all commits
aead3e3
e172c1e
a840be1
a9e17b4
4f81d0a
b2756c9
28b09f4
fd51cfb
377cf3b
3a8f3bf
b5b3d43
594727e
efcf44f
448b191
54c1b57
eb60b1c
b4b355e
20aa7c5
8fa7b09
e5910fa
a7411c8
7de06c1
e50e837
dc2dde4
b7807f9
4344064
0ef4baa
2fa280b
da8cde2
27b2416
b51f965
ef4cd8b
6483da7
4fc7353
7636f78
ea1e525
1698bb8
529caa5
2e190a2
762c7e1
2169579
c2788f6
6c5485f
cb5e47e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
// TODO: Revisit 'max_num_work_group_sync' and align it with the | ||
// 'sycl_ext_oneapi_forward_progress' extension once #7598 is merged. | ||
__SYCL_PARAM_TRAITS_SPEC(ext::oneapi::experimental, kernel_queue_specific, max_num_work_group_sync, size_t,) | ||
__SYCL_PARAM_TRAITS_SPEC(ext::oneapi::experimental, kernel_queue_specific, max_num_work_groups, size_t,) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -159,9 +159,29 @@ class __SYCL_EXPORT kernel : public detail::OwnerLessBase<kernel> { | |
get_info(const device &Device, const range<3> &WGSize) const; | ||
|
||
// TODO: Revisit and align with sycl_ext_oneapi_forward_progress extension | ||
// once #7598 is merged. | ||
// once #7598 is merged. (regarding the 'max_num_work_group_sync' query) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, #7598 has been already merged. Any APIs which do not correspond to root group or launch queries extension should be marked as deprecated together with introduction of a proper API that is documented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the other response wrt this. I see you suggested the introduction of a replacement and deprecating the replaced one go hand-in-hand together. So would you prefer that to happen in separate PR in order for the changes to land in separate squashed commits or you want them in one and just expand on the description? |
||
|
||
/// Query queue/launch-specific information from a kernel using the | ||
/// info::kernel_queue_specific descriptor for a specific Queue. | ||
/// | ||
/// \param Queue is a valid SYCL queue. | ||
/// \return depends on information being queried. | ||
template <typename Param> | ||
typename detail::is_kernel_queue_specific_info_desc<Param>::return_type | ||
ext_oneapi_get_info(queue Queue) const; | ||
|
||
/// Query queue/launch-specific information from a kernel using the | ||
/// info::kernel_queue_specific descriptor for a specific Queue and values. | ||
/// max_num_work_groups is the only valid descriptor for this function. | ||
/// | ||
/// \param Queue is a valid SYCL queue. | ||
/// \param WorkGroupSize is the work-group size the number of work-groups is | ||
/// requested for. | ||
/// \return depends on information being queried. | ||
template <typename Param> | ||
typename Param::return_type ext_oneapi_get_info(const queue &q) const; | ||
typename detail::is_kernel_queue_specific_info_desc<Param>::return_type | ||
ext_oneapi_get_info(queue Queue, const range<3> &WorkGroupSize, | ||
size_t DynamicLocalMemorySize) const; | ||
|
||
private: | ||
/// Constructs a SYCL kernel object from a valid kernel_impl instance. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#7598 has been merged a while ago, this comment should be removed. I also suggest that we deprecate
max_num_work_group_sync
info trait right away in favor ofmax_num_work_groups
. The former is not documented so we should aim to remove it as soon as possible to avoid wide adoption of it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware and was thinking the same. However, I saw it as doing two separate things in one PR that eventually ultimately lands as a single squashed commit and have opted to follow-up with a separate PR solely deprecating
max_num_work_group_sync
. Of course, I am not arguing against your suggestion and preferences. Having said that, let me know how you'd prefer it done. I am fine either way. Thanks!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with doing so in a separate PR to have it a separate commit in our history