-
Notifications
You must be signed in to change notification settings - Fork 769
[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
Conversation
AVALON_STREAMING = 0, | ||
AVALON_STREAMING_USES_READY = 1, | ||
AVALON_MM = 2, | ||
AVALON_MM_USES_READY = 3 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
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.
sycl/doc/extensions/experimental/sycl_ext_intel_data_flow_pipes_properties.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_intel_data_flow_pipes_properties.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_intel_data_flow_pipes_properties.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_intel_data_flow_pipes_properties.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_intel_data_flow_pipes_properties.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_intel_data_flow_pipes_properties.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_intel_data_flow_pipes_properties.asciidoc
Outdated
Show resolved
Hide resolved
sycl/doc/extensions/experimental/sycl_ext_intel_data_flow_pipes_properties.asciidoc
Outdated
Show resolved
Hide resolved
Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
@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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@gmlueck @steffenlarsen Is this okay to be merged now? Ideally the header changes should make it into the 2023.2 release. |
Pipe properties support was added in this PR: #7468 (sycl/ext/intel/experimental/pipe_properties.hpp). This is the accompanying spec.