[SYCL][DOC] Create spec for Pipe Properties extension#9027
[SYCL][DOC] Create spec for Pipe Properties extension#9027steffenlarsen merged 5 commits intointel:syclfrom
Conversation
| AVALON_STREAMING = 0, | ||
| AVALON_STREAMING_USES_READY = 1, | ||
| AVALON_MM = 2, | ||
| AVALON_MM_USES_READY = 3 |
There was a problem hiding this comment.
Upper-case enum values is a little unorthodox for a SYCL enum.
There was a problem hiding this comment.
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.
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.
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.
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.
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). |
|
@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.