Skip to content

[SYCL][DOC] Create spec for Pipe Properties extension #9027

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 5 commits into from
May 2, 2023

Conversation

rho180
Copy link
Contributor

@rho180 rho180 commented Apr 11, 2023

Pipe properties support was added in this PR: #7468 (sycl/ext/intel/experimental/pipe_properties.hpp). This is the accompanying spec.

@rho180 rho180 requested a review from a team as a code owner April 11, 2023 17:13
AVALON_STREAMING = 0,
AVALON_STREAMING_USES_READY = 1,
AVALON_MM = 2,
AVALON_MM_USES_READY = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Upper-case enum values is a little unorthodox for a SYCL enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we already have them in the headers with the upper-case names. @gmlueck - Do you have a preference here? Should we keep the uppercase names or rename it here and in the headers?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that lower case is the norm for SYCL. If we are just developing this extension now, I think it would be better to change to lower case.

If the extension has already been released and customer code is using the upper case spelling, we could just keep that. I think it's somewhat up to the FPGA team. If they want this extension to match stylistically, we could deprecate the upper case spellings and add the lower case spellings now.

Copy link
Contributor Author

@rho180 rho180 Apr 18, 2023

Choose a reason for hiding this comment

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

There's no existing customer code as this is the first release, so I think we're fine with changing the header to use lower case now. Does the header change need to be part of a different PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the header change need to be part of a different PR?

I'm fine with either having it as part of this or as a follow-up patch. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've added the protocol switch to lower-case in the header to this PR. The header also contained a couple of extraneous properties that came out of an earlier draft of the spec, so I removed those as well.

rho180 and others added 2 commits April 12, 2023 09:41
Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
@rho180 rho180 requested review from gmlueck and steffenlarsen April 21, 2023 16:54
@rho180
Copy link
Contributor Author

rho180 commented Apr 21, 2023

@gmlueck @steffenlarsen I re-requested review, wasn't sure if there were other comments, or if you're waiting on me to make changes to the implemented header (or if I should make them in a separate PR).

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!

@rho180 rho180 requested a review from a team as a code owner April 24, 2023 13:19
@rho180 rho180 temporarily deployed to aws April 24, 2023 13:45 — with GitHub Actions Inactive
@rho180 rho180 temporarily deployed to aws April 24, 2023 19:51 — with GitHub Actions Inactive
@rho180 rho180 temporarily deployed to aws April 28, 2023 13:38 — with GitHub Actions Inactive
@rho180 rho180 temporarily deployed to aws April 28, 2023 20:02 — with GitHub Actions Inactive
@rho180
Copy link
Contributor Author

rho180 commented May 2, 2023

@gmlueck @steffenlarsen Is this okay to be merged now? Ideally the header changes should make it into the 2023.2 release.

@steffenlarsen steffenlarsen merged commit 5b0a461 into intel:sycl May 2, 2023
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.

3 participants