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 another parameter value for the pipelined kernel interface property #6966

Merged
merged 7 commits into from
Dec 8, 2022

Conversation

tiwaria1
Copy link
Contributor

@tiwaria1 tiwaria1 commented Oct 5, 2022

The pipelined kernel property is defined to control the pipelining behavior. This PR updates the extension to say how a value of -1 can be used to enforce a particular pipelining behavior.

@tiwaria1 tiwaria1 requested a review from a team as a code owner October 5, 2022 13:44
@@ -287,4 +295,4 @@ q.single_task(KernelFunctor{a, b, c}).wait();
|========================================
|Rev|Date|Author|Changes
|1|2022-03-01|Abhishek Tiwari|*Initial public working draft*
Copy link
Contributor

Choose a reason for hiding this comment

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

Update date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed as that was the date when the initial spec was put for review.

When the parameter is set to `0`, the compiler will not pipeline kernel
invocations.

If the `pipelined` property is not specified, the default behavior is
equivalent to a combination of the property parameter values listed above, but
the choice is implementation defined.
the choice is implementation defined. Similarly, if the property parameter is
not specified, the default value is implementation defined.
Copy link
Contributor

@GarveyJoe GarveyJoe Oct 5, 2022

Choose a reason for hiding this comment

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

If the parameter is not defined I think we should say its default value is -1. -1 already means it's up to the compiler to determine II so I don't think we need the extra layer of implementation defined indirection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

tiwaria1 and others added 3 commits December 6, 2022 10:27
Co-authored-by: Joe Garvey <joseph.garvey@intel.com>
Co-authored-by: Joe Garvey <joseph.garvey@intel.com>
@tiwaria1
Copy link
Contributor Author

tiwaria1 commented Dec 6, 2022

@intel/dpcpp-specification-reviewers Please review and merge this one, thank you!

When the parameter is set to `0`, the compiler will not pipeline kernel
invocations.

If the `pipelined` property is not specified, the default behavior is
equivalent to a combination of the property parameter values listed above, but
the choice is implementation defined.

If the property parameter is not specified, the default value is `-1`.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if N < -1? If it isn't a valid value, could you please add a note somewhere in this section specifying that valid values are N >= -1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the quick review. Updated to state valid values are >= -1.

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.

LGTM apart from the one comment.

@tiwaria1
Copy link
Contributor Author

tiwaria1 commented Dec 7, 2022

LGTM apart from the one comment.

Addressed. Please merge when possible. Thank you! :)

@steffenlarsen steffenlarsen merged commit 1a1fd8d into intel:sycl Dec 8, 2022
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.

4 participants