Skip to content

[SYCL][Doc] Add KernelProperties extension #4280

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

Merged
merged 12 commits into from
Oct 19, 2021

Conversation

Pennycook
Copy link
Contributor

This extension introduces a replacement for the kernel attributes
defined in Section 5.8.1 of the SYCL 2020 specification, in the form of
a property_list accepting properties with compile-time constant
values.

Signed-off-by: John Pennycook [email protected]

@Pennycook Pennycook added spec extension All issues/PRs related to extensions specifications Documentation Missing documentation for the code, compiler or runtime features, etc. labels Aug 6, 2021
This extension introduces a replacement for the kernel attributes
defined in Section 5.8.1 of the SYCL 2020 specification, in the form of
a property_list accepting properties with compile-time constant
values.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook
Copy link
Contributor Author

This extension builds on #4203, and lays the groundwork for additional properties that may affect kernel launch and compilation (e.g. cache sizes from #3416, additional sub-group sizes and forward progress guarantees). Its design is intended to address #1254.

Tagging @keryell and @illuhad for feedback.

Copy link
Contributor

@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.

Quite interesting.
I propose some shorter implementation by hiding the concrete property types.

Implementing the PROPERTIES macro as previously described would have
required a very complex solution (or usage of a preprocessor library).
Additionally, forgetting to enclose arguments in parentheses would
produce error messages that are difficult to understand.

Providing a PROPERTY macro that a developer may supply multiple times
is much simpler to implement and gives much better error messages.
class queue {
public:
template <typename KernelName, typename KernelType, typename PropertyList>
event single_task(PropertyList properties, const KernelType &kernelFunc);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the spec ask us to have a prefix ext_oneapi_ here?
The only section which may tell that this is an expectation is:

A similar situation can occur if an existing SYCL template is
specialized with an extended enumerated value. Obviously,
the extension cannot rename the template in this case.
Instead, it is sufficient that the template is specialized with
an extended enumerated value, and this guarantees that
the extended specialization will not collide.

but I'm not sure I can tell for sure what it is talking about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we're following the guidelines in the paragraph above that one:

In some cases, an extension does not have the freedom to choose a specific function name. For example, this could happen if the extension adds a new constructor overload for an existing SYCL class. In cases like this, the extension should ensure that one of the function parameters has a type that is defined in the extension’s namespace. For example, the Acme vendor could add a new constructor for sycl::context with the signature context(ext::acme::frobber &).

We're adding an argument to single_task, parallel_for etc, but that argument is in our namespace (ext::oneapi::property_list); if a developer tries to use this extension with another SYCL compiler, they'll get an error about ext::oneapi::property_list not being defined, and from that it will be obvious that they're trying to use an extension.

Arguably we do have the freedom to choose a different function name in this case, but I think the proposed syntax is easier to remember and still in line with the guidelines.

Tagging @gmlueck for awareness, and in case he has anything to add.

@Pennycook Pennycook marked this pull request as ready for review October 6, 2021 14:58
@Pennycook Pennycook requested a review from a team as a code owner October 6, 2021 14:58
@Pennycook
Copy link
Contributor Author

Since #4203 has been merged, I'm marking this ready for review.

@Pennycook
Copy link
Contributor Author

Pinging @intel/dpcpp-specification-reviewers; besides the merge conflict, is there anything more we need to do here?

Copy link
Contributor

@gmlueck gmlueck left a comment

Choose a reason for hiding this comment

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

Just a couple small nits I noticed on re-reading.

@bader bader merged commit 64f5e70 into intel:sycl Oct 19, 2021
@Pennycook Pennycook deleted the kernel-properties branch November 11, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Missing documentation for the code, compiler or runtime features, etc. spec extension All issues/PRs related to extensions specifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants