Skip to content

Conversation

Aympab
Copy link
Contributor

@Aympab Aympab commented Mar 21, 2025

The CTS for the khr extension proposal for the device descriptor max_num_work_groups.

There is a PR for an implementation in AdaptiveCpp.

@Aympab Aympab requested a review from a team as a code owner March 21, 2025 14:40
@CLAassistant
Copy link

CLAassistant commented Mar 21, 2025

CLA assistant check
All committers have signed the CLA.

@keryell
Copy link
Member

keryell commented Mar 26, 2025

Formatting to be fixed?

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thanks!
A few remarks.

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines 35 to 43
return;
auto max_work_groups =
dev.get_info<khr::info::device::max_num_work_groups<DIMENSION>>();

for (int i = 0; i < DIMENSION; i++) {
CHECK(max_work_groups[i] >= 1);
}
}
}
Copy link
Contributor

@bader bader Apr 9, 2025

Choose a reason for hiding this comment

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

Suggested change
return;
auto max_work_groups =
dev.get_info<khr::info::device::max_num_work_groups<DIMENSION>>();
for (int i = 0; i < DIMENSION; i++) {
CHECK(max_work_groups[i] >= 1);
}
}
}
return;
auto max_work_groups =
dev.get_info<khr::info::device::max_num_work_groups<DIMENSION>>();
for (int i = 0; i < DIMENSION; i++) {
CHECK(max_work_groups[i] >= 1);
}
}

One } seems to be redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add an option to https://github.com/KhronosGroup/SYCL-CTS/blob/main/CMakeLists.txt and build locally with any implementation that supports it. I suspect there are some syntax errors in the current version.

add_cts_option(SYCL_CTS_ENABLE_MAX_NUM_WORK_GROUPS
    "Enable ..." OFF
    FORCE_ON ${SYCL_CTS_ENABLE_KHR_TESTS})

Copy link
Contributor Author

@Aympab Aympab Apr 22, 2025

Choose a reason for hiding this comment

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

Okay done, I fixed a few syntax issues.
I compiled the CTS with acpp from this PR which supports the extension (removing most of the other CTS tests).

@tomdeakin
Copy link
Contributor

one more review required after resolving above comments.

@keryell keryell self-requested a review April 22, 2025 01:23
@@ -0,0 +1,5 @@
if(SYCL_CTS_ENABLE_MAX_NUM_WORK_GROUPS)
Copy link
Member

Choose a reason for hiding this comment

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

I am confused. Should it be SYCL_CTS_ENABLE_KHR_MAX_NUM_WORK_GROUPS instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a bug vs line 130

"the max_num_work_groups extension defines the "
"SYCL_KHR_MAX_NUM_WORK_GROUPS macro",
"[khr_max_num_work_groups]") {
#ifndef SYCL_KHR_MAX_NUM_WORK_GROUPS
Copy link
Member

Choose a reason for hiding this comment

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

Where/how is this macro set?

Copy link
Contributor Author

@Aympab Aympab Apr 26, 2025

Choose a reason for hiding this comment

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

This macro is set by the SYCL implementation
It's a requirement when the extension is implemented, the macro should be defined
(See https://github.com/Aympab/SYCL-Docs/blob/khr-max-num-wg/adoc/extensions/sycl_khr_max_num_work_groups.adoc#L15 )

@keryell keryell self-requested a review April 26, 2025 00:36
Co-authored-by: Ronan Keryell <[email protected]>
@bader bader requested a review from tomdeakin May 29, 2025 15:06
@Aympab
Copy link
Contributor Author

Aympab commented Jun 11, 2025

I've updated the PR following the discussion in KhronosGroup/SYCL-Docs#712

@Aympab Aympab changed the title [KHR] Tests for max_num_work_groups extension [KHR] Tests for sycl_khr_max_work_group_queries extension Jun 11, 2025
@tomdeakin
Copy link
Contributor

WG approved to merge

@tomdeakin tomdeakin merged commit 4946ff2 into KhronosGroup:main Jun 19, 2025
9 checks passed
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.

5 participants