Skip to content

Implement sycl_khr_work_item_queries extension #18519

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

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

Conversation

0x12CC
Copy link
Contributor

@0x12CC 0x12CC commented May 16, 2025

Implements the extension defined in KhronosGroup/SYCL-Docs#682.

@0x12CC 0x12CC requested a review from a team as a code owner May 16, 2025 17:18
@0x12CC 0x12CC requested a review from steffenlarsen May 16, 2025 17:18
@0x12CC 0x12CC temporarily deployed to WindowsCILock May 16, 2025 17:51 — with GitHub Actions Inactive
@0x12CC 0x12CC temporarily deployed to WindowsCILock May 16, 2025 18:51 — with GitHub Actions Inactive
@0x12CC 0x12CC temporarily deployed to WindowsCILock May 16, 2025 18:51 — with GitHub Actions Inactive
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Looks good overall! One test nit.


#include <sycl/ext/oneapi/free_function_queries.hpp>

#define SYCL_KHR_WORK_ITEM_QUERIES 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is how we do it in khr/group_interface.hpp too, but would it be more appropriate to guard the definition in sycl/source/feature_test.hpp.in together with all the other feature test macros?

Comment on lines +36 to +37
for (const auto &result : acc)
assert(result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit; Asserts stop execution immediately, so personally I prefer to do something like:

Suggested change
for (const auto &result : acc)
assert(result);
for (const auto &result : acc) {
if (!result) {
std::cout << "Failed check_this_nd_item_api check for dimensionality " << Dimensions << ".\n";
return 1;
}
}
return 0;

and then in the main function you do:

  int Failed = 0;
  // nd_item
  Failed += check_this_nd_item_api<2>();
  Failed += check_this_nd_item_api<2, 3>();
  Failed += check_this_nd_item_api<2, 3, 4>();
  // group
  Failed += check_this_group_api<2>();
  Failed += check_this_group_api<2, 3>();
  Failed += check_this_group_api<2, 3, 4>();
  // sub_group
  Failed += check_this_sub_group_api<2>();
  Failed += check_this_sub_group_api<2, 3>();
  Failed += check_this_sub_group_api<2, 3, 4>();

  return Failed;

The more information, the better.

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.

2 participants