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

Add test plan for sycl_ext_oneapi_enqueue_functions extension #898

Merged

Conversation

0x12CC
Copy link
Contributor

@0x12CC 0x12CC commented May 31, 2024

No description provided.

@0x12CC 0x12CC requested review from gmlueck and a team as code owners May 31, 2024 14:23
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.

It would be good to also have static_assert on the return type of all of these functions, but other than that I think this looks reasonable.

0x12CC added a commit to 0x12CC/SYCL-CTS that referenced this pull request Jun 13, 2024
@bader bader requested a review from keryell June 14, 2024 18:05
@bader
Copy link
Contributor

bader commented Jun 14, 2024

@gmlueck, ping.

@gmlueck
Copy link
Contributor

gmlueck commented Jun 14, 2024

@gmlueck, ping.

I trust @steffenlarsen's review. Is there a specific question you would like me to answer?

@bader
Copy link
Contributor

bader commented Jun 14, 2024

@gmlueck, ping.

I trust @steffenlarsen's review. Is there a specific question you would like me to answer?

You are the code owner of test plans for extensions, so formally either you or SYCL implementation representatives (i.e. @KhronosGroup/sycl-cts-reviewers team) have to approve test plans.
I'm really appreciate the efforts @steffenlarsen is making to push SYCL-CTS PRs forward, so I suggest we make him a formal reviewer of SYCL-CTS code and/or test plans.

@gmlueck
Copy link
Contributor

gmlueck commented Jun 14, 2024

I'm really appreciate the efforts @steffenlarsen is making to push SYCL-CTS PRs forward, so I suggest we make him a formal reviewer of SYCL-CTS code and/or test plans.

Yes, I agree.

@keryell
Copy link
Member

keryell commented Jun 18, 2024

@gmlueck It could be interesting if these functions were able to be found via ADL to avoid the namespace, for example if the extension header is used.

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.

That looks good.

@gmlueck
Copy link
Contributor

gmlueck commented Jun 18, 2024

@gmlueck It could be interesting if these functions were able to be found via ADL to avoid the namespace, for example if the extension header is used.

In order to do this, I think we would need to define these functions in the sycl namespace instead of the sycl::ext::oneapi namespace. However, this would be contrary to our goal of exposing extended APIs only via an "extension" namespace. Therefore, we would probably not do this. However, if this extension was adopted into a future version of the core spec, then it would be defined in the sycl namespace, and it would therefore be available via ADL.

@bader bader merged commit 7225b94 into KhronosGroup:SYCL-2020 Jun 18, 2024
8 checks passed
@0x12CC 0x12CC deleted the oneapi_enqueue_functions_test_plan branch June 18, 2024 16:29
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