Skip to content

Conversation

@Pennycook
Copy link
Contributor

Adopt the longer Constraints/Effects/Returns format from ISO C++, which clarifies how the different overloads are intended to work.

Adopt the longer Constraints/Effects/Returns format from ISO C++,
which clarifies how the different overloads are intended to work.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook Pennycook added the spec extension All issues/PRs related to extensions specifications label Nov 7, 2024
@Pennycook Pennycook requested a review from gmlueck November 7, 2024 16:40
@Pennycook Pennycook requested a review from a team as a code owner November 7, 2024 16:40
class kernel {
public:
template <typename Param, typename... T>
/*return-type*/ ext_oneapi_get_info(T... args) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to define this as a series of overloads rather than using a parameter pack. The general pattern of this API is similar to device::create_sub_devices, and we used overloads there. Using overloads should result in better error messages if the application passes a wrong parameter, and I think it will be easier to implement also. For example:

template <typename Param, uint32_t Dimensions>
id<Dimensions> ext_oneapi_get_info(sycl::queue q) const;

template <typename Param>
size_t ext_oneapi_get_info(sycl::queue q) const;

template <typename Param>
uint32_t ext_oneapi_get_info(sycl::queue q, sycl::range<1> r) const;

template <typename Param>
uint32_t ext_oneapi_get_info(sycl::queue q, sycl::range<2> r) const;

template <typename Param>
uint32_t ext_oneapi_get_info(sycl::queue q, sycl::range<3> r) const;

template <typename Param>
uint32_t ext_oneapi_get_info(sycl::queue q, sycl::range<1> r) const;

template <typename Param>
uint32_t ext_oneapi_get_info(sycl::queue q, sycl::range<2> r) const;

template <typename Param>
uint32_t ext_oneapi_get_info(sycl::queue q, sycl::range<3> r) const;

Each function would still have a constraint on Param as you show below.

Copy link
Contributor Author

@Pennycook Pennycook Jan 9, 2025

Choose a reason for hiding this comment

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

They are defined as separate overloads. The parameter pack here was just intended to serve as a stub to document at a high-level what the query functions do, without listing all of the overloads in the synopsis.

If you'd prefer that we list all the overloads, I'm not opposed to that: done in 03229e1. Note that I added "Only available if" comments because otherwise it's not clear how the overloads could coexist.

Aside: Making these changes has made me wonder why we have a unified query interface at all. The specification is pretty ugly. Something like k.get_max_work_item_sizes<3>(q) would be much easier to describe, and is also significantly less verbose than k.ext_oneapi_get_info<sycl::ext::info::kernel::max_work_item_sizes<3>>(q). I'm not suggesting we do anything in this PR, but if you agree then it might be worth raising at Khronos.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I think this is better than the parameter pack.

I'm not sure why the SYCL style is to have a template parameter for these queries, rather than just having different function names. Maybe @AerialMantis knows the history.

Possibly, it was done this way to make the spec less verbose. For the version of get_info with no parameters, there is just one function template:

template <typename Param>
typename Param::return_type get_info() const;

This works because the return type is defined (indirectly) by the template parameter. In your case, there can't be just one function template because the types of the parameters cannot be defined by the template parameter.

However, even though the spec is less verbose (for the case when get_info has no parameters), code is still more verbose than having separate functions. Therefore, I'm not sure this is a good rationale for using the template approach.

@Pennycook Pennycook force-pushed the sycl_ext_oneapi_launch_queries branch from 6958b71 to 3ccbfa7 Compare January 9, 2025 09:18
@Pennycook
Copy link
Contributor Author

@intel/llvm-gatekeepers, I think this can be merged. The unresolved question from Greg above is orthogonal to the changes here -- if we decided to change the format of all SYCL queries, I suspect we'd do it as part of a different extension.

@dm-vodopyanov dm-vodopyanov merged commit 2fb0cb3 into intel:sycl Jan 20, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spec extension All issues/PRs related to extensions specifications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants