[SYCL][Doc] Update SYCL_INTEL_data_flow_pipes extension for FPGA host pipe support #5838
Conversation
… pipe support Add a memory order parameter to device-side read/write members and default to `sycl::memory_order::seq_cst`. Replace `min_capacity` property with compile-time properties list for use with `SYCL_INTEL_FPGA_data_flow_pipes_properties` extension. Add host pipe read/write members with additional `sycl::queue` parameter. Signed-off-by: Peter Colberg <peter.colberg@intel.com>
| @@ -116,7 +130,7 @@ A pipe type is a specialization of the pipe class: | |||
| ---- | |||
| template <typename name, | |||
| typename dataT, | |||
| size_t min_capacity = 0> | |||
| typename propertiesT = properties<>> | |||
There was a problem hiding this comment.
Using properties here is nice, but this is an API-breaking change to a supported extension. Existing code using the old API will no longer compiler, right?
There was a problem hiding this comment.
Its not possible to keep both class variant at the same time in experimental.
// existing implementation
template <class _name, class _dataT, int32_t _min_capacity = 0>
class pipe {
};
// updated implementation
template <class _name, class _dataT, typename PropertyList>
class pipe {
};Will give error:
./Playground/file0.cpp:4:62: error: template parameter 'int _min_capacity'
4 | template <class _name, class _dataT, int32_t _min_capacity = 0>
| ^
./Playground/file0.cpp:10:7: note: redeclared here as 'class PropertyList'
10 | class pipe {
| ^~~~
The choice is between: breaking ABI vs defining a new name/namespace for pipes
There was a problem hiding this comment.
This is exactly the difference between an "experimental" extension vs. a "supported" one. If it is "supported", we promise not to break API compatibility with existing code without going through a deprecation process.
If the FPGA team wants the freedom to change APIs without deprecation, we should consider making this an experimental API (which includes moving it to the the experimental namespace).
There was a problem hiding this comment.
Indeed this breaks the ABI, and I don't see any trick using SFINAE via std::enable_if to retain compatibility.
@GarveyJoe, am I missing anything obvious?
@gmlueck, would you accept a global macro to opt pipe into the properties parameter, along with a deprecation notice for the _min_capacity parameter when the macro is undefined? The properties parameter would become the default at some later point, after compile-time constant properties have been moved out of experimental.
There was a problem hiding this comment.
This is exactly the difference between an "experimental" extension vs. a "supported" one. If it is "supported", we promise not to break API compatibility with existing code without going through a deprecation process.
If the FPGA team wants the freedom to change APIs without deprecation, we should consider making this an experimental API (which includes moving it to the the
experimentalnamespace).
Yes, this feature would be implemented in sycl::ext::intel::experimental::pipe for now, but that still leaves the question of how to allow for a deprecation period in which both the old and the new template parameter are supported once this is moved out of experimental.
There was a problem hiding this comment.
@pcolberg @GarveyJoe @gmlueck Is there anything we still need to resolve in this conversation?
There was a problem hiding this comment.
The conclusion was to retain _min_capacity as the third parameter and add compile-time properties as a fourth parameter, which has been merged in #5886 for the experimental namespace.
sycl/doc/extensions/proposed/sycl_ext_intel_dataflow_pipes.asciidoc
Outdated
Show resolved
Hide resolved
To align with spec: intel#5838 Host pipe is a FIFO construct that provide links between elements of design that are accessed through read/write API. Host pipe is a pipe that links device kernel with host program.
Defines new properties for data flow pipes Properties defined to align with spec in [1] Existing data flow pipe defined in [2] data flow pipe will be accepting new property list, spec in [3] [1] intel#5839 [2] https://github.com/intel/llvm/blob/sycl/sycl/doc/extensions/proposed/sycl_ext_intel_dataflow_pipes.asciidoc [3] intel#5838
Setup lower runtime extension functions for host pipes. See also intel#5766 intel#5851 Host pipe sycl spec: intel#5838
What is this for: pipes expose the concept of a first in first out buffer, this FIFO construct provide link between elements of a design that are accessed through read/write/push/pop APIs. A host pipe is a pipe that links a device kernel with host program. This extension is framed from FPGA perspective. This change add required interface for the integration footer to register the `host_pipe` of a program as well as reading extended info supplied through "SYCL/host pipes" property. Info is stored in a map managed by program manager. The integration header and footer provides a mapping from the host address of each pipe variable to the unique string for that variable. This is required so that sycl runtime can query the pipe address from the given pipe name, and pass both into opencl runtime function calls. Opencl defines pipes, which are FIFO constructs that are consistent with Khronos specification. Spec link: intel#5838 Note: it is the first change to runtime relating to host_pipe, thus the feature is not complete / fully testable. It is intended to add an interface for integration footer as well as consumer for the information sycl-post-link will be generating when future work is added. (cherry picked from commit 031f829) Zibai fixed all conflicts.
Setup lower runtime extension functions for host pipes. See also intel#5766 intel#5851 Host pipe sycl spec: intel#5838 (cherry picked from commit e4d513c) Zibai fixed the conflict.
Defines the flow of enqueue new host pipe operations (read/write), User provide the queue to enqueue this event, and the runtime queries the pipe address from registration using the given address and unique ID. The runtime pass the pipe name, and host address into queue submit of new command group. The enqueued command calls new opencl function, and provide the current program, queue, event wait list, pipe name, host pointer of the data destination. Spec: intel#5838 (cherry picked from commit 12e9e85) Zibai fixed all the conflicts.
What is this for: pipes expose the concept of a first in first out buffer, this FIFO construct provide link between elements of a design that are accessed through read/write/push/pop APIs. A host pipe is a pipe that links a device kernel with host program. This extension is framed from FPGA perspective. This change add required interface for the integration footer to register the `host_pipe` of a program as well as reading extended info supplied through "SYCL/host pipes" property. Info is stored in a map managed by program manager. The integration header and footer provides a mapping from the host address of each pipe variable to the unique string for that variable. This is required so that sycl runtime can query the pipe address from the given pipe name, and pass both into opencl runtime function calls. Opencl defines pipes, which are FIFO constructs that are consistent with Khronos specification. Spec link: intel#5838 Note: it is the first change to runtime relating to host_pipe, thus the feature is not complete / fully testable. It is intended to add an interface for integration footer as well as consumer for the information sycl-post-link will be generating when future work is added. (cherry picked from commit 031f829) Zibai fixed all conflicts.
Setup lower runtime extension functions for host pipes. See also intel#5766 intel#5851 Host pipe sycl spec: intel#5838 (cherry picked from commit e4d513c) Zibai fixed the conflict.
Defines the flow of enqueue new host pipe operations (read/write), User provide the queue to enqueue this event, and the runtime queries the pipe address from registration using the given address and unique ID. The runtime pass the pipe name, and host address into queue submit of new command group. The enqueued command calls new opencl function, and provide the current program, queue, event wait list, pipe name, host pointer of the data destination. Spec: intel#5838 (cherry picked from commit 12e9e85) Zibai fixed all the conflicts.
|
This pull request was closed because it has been stalled for 30 days with no activity. |
#8789) A continuation of #5838. Accompanying runtime change is #7468. Add a memory order parameter to device-side read/write members and default to sycl::memory_order::seq_cst. This parameter is in place but not being used at this moment, it's intended for the future work. Add host pipe read/write members with additional sycl::queue parameter. --------- Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
Add a memory order parameter to device-side read/write members and
default to
sycl::memory_order::seq_cst.Replace
min_capacityproperty with compile-time properties list foruse with
SYCL_INTEL_FPGA_data_flow_pipes_propertiesextension.Add host pipe read/write members with additional
sycl::queueparameter.Cc: @mkinsner @GarveyJoe @aditikum @rho180 @zibaiwan